Bug 222257 - Review Request: pastebin - A collaborative debugging tool
Review Request: pastebin - A collaborative debugging tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dennis Gilmore
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2007-01-11 00:41 EST by Michael Stahnke
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-12 16:29:06 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Michael Stahnke 2007-01-11 00:41:30 EST
Spec URL: http://www.stahnkage.com/rpms/pastebin.spec
SRPM URL: http://www.stahnkage.com/rpms/pastebin-0.50-1.src.rpm
Description: pastebin is here to help you collaborate on debugging code snippets. 
If you're not familiar with the idea, most people use it to submit a code
fragment to pastebin, getting a url like http://pastebin.com/1234 and then
link that URL in IRC or IM conversations.  This allows others to see your
code and optionally post changes.
Comment 1 Dennis Gilmore 2007-01-11 01:21:43 EST
 package meets naming and packaging guidelines.
 specfile is properly named, is cleanly written and uses macros consistently.
 dist tag is present.
 build root is correct.
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 license field matches the actual license.
 license is open source-compatible.  GPL License text included in package.
 source files match upstream:
      d7b8993f4baed7753fb7c912b06725fb  pastebin.tar.gz
      d7b8993f4baed7753fb7c912b06725fb  ../SOURCES/pastebin.tar.gz
 latest version is being packaged.
 BuildRequires are proper.
 package builds in mock ( FC-6 ).
 rpmlint is silent.
 final provides and requires are sane
 no shared libraries are present.
 package is not relocatable.
 doesn't own any directories it shouldn't.
 file permissions are appropriate.
 %clean is present.
 no scriptlets present.
 code, not content.
 documentation is small, so no -docs subpackage is necessary.
 %docs are not necessary for the proper functioning of the package.
 no headers.
 no pkgconfig files.
 no libtool .la droppings.
 not a GUI app.

Needs fixing 
does not own %{_datadir}/%{name}
%{_sysconfdir}/%{name} listed twice
Comment 2 Michael Stahnke 2007-01-11 10:31:41 EST
I have fixed he above issues. 

Spec URL: http://www.stahnkage.com/rpms/pastebin.spec
SRPM URL: http://www.stahnkage.com/rpms/pastebin-0.50-2.src.rpm
Comment 3 Dennis Gilmore 2007-01-11 10:41:42 EST
ok looks good and the package works.

APPROVED

and ill sponsor you also  as it seems you are not sponsored already 

apply for cvsextras group access
Comment 4 Michał Bentkowski 2007-01-11 11:08:32 EST
(In reply to comment #1)
>  rpmlint is silent.

rpmlint is not silent:
W: pastebin mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)

Also, there are some documentation files in %{_datadir}/pastebin/lib/geshi/docs
which should be moved to %doc. Creation of new directory there, like geshi
sounds like a good solution. It seems to me that the content of 
{_datadir}/pastebin/lib/geshi may be moved there as well, obviously after some
fixing in example.php.
Comment 5 Michał Bentkowski 2007-01-11 11:10:55 EST
Oops, I didn't want to set an FE-REVIEW blocker back. Changing to FE-ACCEPT
again.
Comment 6 Michael Stahnke 2007-01-11 11:27:19 EST
Are you saying the geshi doc should be in /usr/share/doc/geshi or
/usr/share/doc/pastebin-0.50/geshi?
Comment 7 Mamoru TASAKA 2007-01-11 11:32:12 EST
Well, some notes:

Keep timestamps for text files as possible.
( Timestamps in http://fedoraproject.org/wiki/Packaging/Guidelines )

* Use "cp -p" or "install -p" instead of just "cp" or "install"
* for sed usage:
----------------------------------------------------
find . -type f| xargs sed -i 's/\r//' 
----------------------------------------------------
  Well, this usage of find -> sed change timestamps of all files
  under the directory ".", even some (many) files are actually
  not necessary to be fixed.

  Just use "sed" to the files which are _actually_ needed to
  be changed.
Comment 8 Michał Bentkowski 2007-01-11 11:35:40 EST
(In reply to comment #6)
> Are you saying the geshi doc should be in /usr/share/doc/geshi or
> /usr/share/doc/pastebin-0.50/geshi?
> 

I meant the second one.
Comment 9 Dennis Gilmore 2007-01-11 11:49:06 EST
with the find and sed removed i get 
[dennis@daedalus SPECS]$ 
rpmlint /home/dennis/fedora/RPMS/noarch/pastebin-0.50-2.noarch.rpm |grep 
wrong-script-end-of-line-encoding |wc -l
84
[dennis@daedalus SPECS]$ 
rpm -qlp /home/dennis/fedora/RPMS/noarch/pastebin-0.50-2.noarch.rpm |wc -l
106
[dennis@daedalus SPECS]$

so nearly every single file is dos line ended  in this instance i think it is 
fine to mass change line endings.  if it was a small handfull of files i would 
do them individually. but that is not the case.

as far as the docs  yeah I missed that  they should go into %doc
Comment 10 Dennis Gilmore 2007-01-11 11:53:23 EST
as far as rpmlint goes 
[dennis@daedalus SPECS]$ 
rpmlint /home/dennis/fedora/RPMS/noarch/pastebin-0.50-2.noarch.rpm
[dennis@daedalus SPECS]$ 
rpmlint /home/dennis/fedora/RPMS/noarch/pastebin-0.50-1.noarch.rpm
[dennis@daedalus SPECS]$ 

it is indeed silent for me if there is mixed spaces/tabs  then thats should be 
fixed 
Comment 11 Michał Bentkowski 2007-01-11 12:12:41 EST
(In reply to comment #10)
> it is indeed silent for me if there is mixed spaces/tabs  then thats should 
be 
> fixed 

Apart from built rpm file, you ought to also check rpmlint against source rpm
file.
[ecik@ecik ~]$ rpmlint ~/rpmbuild/SRPMS/pastebin-0.50-2.src.rpm
W: pastebin mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)
Comment 12 Mamoru TASAKA 2007-01-11 12:15:15 EST
(In reply to comment #9)
> with the find and sed removed i get 
> [dennis@daedalus SPECS]$ 
> rpmlint /home/dennis/fedora/RPMS/noarch/pastebin-0.50-2.noarch.rpm |grep 
> wrong-script-end-of-line-encoding |wc -l
> 84
> [dennis@daedalus SPECS]$ 
> rpm -qlp /home/dennis/fedora/RPMS/noarch/pastebin-0.50-2.noarch.rpm |wc -l
> 106
> [dennis@daedalus SPECS]$
> 
> so nearly every single file is dos line ended  in this instance i think it is 
> fine to mass change line endings.  if it was a small handfull of files i would 
> do them individually. but that is not the case.

Well, even this case, still checking if the file 
"sed" command is to be applied is really a text file.
As far as I checked, there is one file which is not a text file
(./public_html/favicon.ico),
against this file "sed" command should not be used.
Comment 13 Michał Bentkowski 2007-01-11 12:40:37 EST
(In reply to comment #12)
> Well, even this case, still checking if the file 
> "sed" command is to be applied is really a text file.
> As far as I checked, there is one file which is not a text file
> (./public_html/favicon.ico),
> against this file "sed" command should not be used.

It may be easily fixed by sedding only the files in lib/ subdirectory.
Comment 14 Michał Bentkowski 2007-01-11 13:15:37 EST
I forgot to add that the files in public_html/ directory should be listed
explicitly.
Comment 15 Michael Stahnke 2007-01-11 14:15:53 EST
Can you tell me why (source) I should list public_html files explicitly?  I
didn't see anything on http://fedoraproject.org/wiki/Packaging/Guidelines about it. 

I am not saying I won't do it, I just want to know why. 

Comment 16 Michał Bentkowski 2007-01-11 14:26:36 EST
(In reply to comment #15)
> Can you tell me why (source) I should list public_html files explicitly?
> 

I meant that you ought to sed all files in lib/ and explicit list files to sed
in public_html.
Comment 17 Michael Stahnke 2007-01-11 15:05:01 EST
Would this be an acceptable way to sed only text files? 


for file in `find . -type f`
do
   if (file $file | awk -F: '{print $2}' | grep -i text &> /dev/null) ; then
      sed -i 's/\r//g' $file
   fi
done
Comment 19 Mamoru TASAKA 2007-01-12 12:23:39 EST
(In reply to comment #17)
> Would this be an acceptable way to sed only text files? 

This script should work well.
I can accept 0.50-3 for sed issue (not checked if other issues
exist, however I assume it is okay for now).

Note You need to log in before you can comment on or make changes to this bug.