Bug 506755

I should mention this this passes rpmlint, builds in i386 and x86_64 mock chroots, and also passed a scratch build in koji.

Thanks in advance.

Hi !

Although i cannot sponsor you yet, I've reviewed your package. Here are some comments:
a. Change the 'Source' tag URL to point to the correct upstream source URL[1]:
-Source0:        http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
+Source0:        http://downloads.sourceforge.net/%{name}-%{version}.tar.gz

b. rpmlint on the binary rpm returns a warning:
[steve@laptop SPECS]$ rpmlint -i /home/steve/rpmbuild/RPMS/x86_64/tmux-0.8-1.fc10.x86_64.rpm
tmux.x86_64: W: spurious-executable-perm /usr/share/man/man1/tmux.1.gz
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.
[steve@laptop SPECS]$

So, you'd want to modify the tmux-0.8-fixperms.patch to keep the permissions settings as they are and may be just remove the explicit ownership settings.
    
c. It is preferable to not wildcard everything in %files in such a general manner. You may want to be specific for at least the %{_bindir} (since it is only 1 file each for bindir and mandir, there is no reason why you should have wildcards).

Also, I'll set the FE-NEEDSPONSOR in the bug blocks[2]

cheers,
- steve
[1] https://fedoraproject.org/wiki/Packaging/SourceURL#Referencing_Source
[2] https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Create_Your_Review_Request

sorry, in point (a), it should've been:
-Source0: http://downloads.sourceforge.net/%{name}-%{version}.tar.gz
+Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz

you get the idea right? :)
gah !
- steve

Steve,

Many thanks for the review and the helpful comments.  I will revise the spec to address the issues you identified.  And thanks also for setting the FE-NEEDSPONSOR.  I saw that in the guides, but was a bit unsure exactly what to do there.

Cheers!

I believe I have addressed the issues Steve identified.  Here are links to the updated Spec and SRPM[1]:

Spec URL: http://chessgriffin.com/files/pkgs/fedora/tmux/tmux.spec
SRPM URL: http://chessgriffin.com/files/pkgs/fedora/tmux/tmux-0.8-2.fc11.src.rpm

On the man page permissions, the Makefile sets those to 444, but in the tmux-0.8-fixperms.patch I modfied it to 644 to match what appears to be the prevailing standard in /usr/share/man/man1.  As a result of the updates, I bumped the release to 2 and updated the changlog.

I checked rpmlint -i on the spec, RPM, and SRPM and had no warnings or errors.  I also built in mock with f11-x86_64 and f11-i386 chroot with no errors.  Finally, I tested this updated release on koji and it appears to build fine:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1426777

Thanks!

[1] The old Spec and SRPM can be found at:
http://chessgriffin.com/files/pkgs/fedora/tmux/old

- If you need to redefine the prefix and mandir then the makefile probably doesn't support DESTDIR, which should be fixed in the first place.

- You don't need a patch to set the man page permissions, you can just chmod it after the install. (You can contact upstream about them, though.)

- What happens if you don't use patch2? Doesn't -I work as well as -iquote..?

Hello Jussi!  Thank you for your comments.

"- If you need to redefine the prefix and mandir then the makefile probably
doesn't support DESTDIR, which should be fixed in the first place."

It does support DESTDIR, but sets PREFIX to /usr/local.  Here is a snippet from the unpatched makefile:

install:
        $(INSTALLDIR) $(DESTDIR)$(PREFIX)/bin
        $(INSTALLBIN) $(PROG) $(DESTDIR)$(PREFIX)/bin/$(PROG)
        $(INSTALLDIR) $(DESTDIR)$(PREFIX)/man/man1
        $(INSTALLMAN) $(PROG).1 $(DESTDIR)$(PREFIX)/man/man1/$(PROG).1

So I think this patch is necessary to insert $MANDIR in the relevant places and then PREFIX and MANDIR can be defined at make install.  The spec uses DESTDIR=$RPM_BUILD_ROOT too.

"- You don't need a patch to set the man page permissions, you can just chmod it
after the install. (You can contact upstream about them, though.)"

That patch also removes some ownership bits as well:

 PREFIX?= /usr/local
 INSTALLDIR= install -d
-INSTALLBIN= install -g bin -o root -m 555
-INSTALLMAN= install -g bin -o root -m 444
+INSTALLBIN= install -p
+INSTALLMAN= install -p -m 644

Perhaps the patch should remove the ownership/permission bits entirely and like you say I can chmod the man page to 644 -- I guess I would do that in %files? 

"- What happens if you don't use patch2? Doesn't -I work as well as -iquote..?"

It fails to build in i386 and PPC.  Some other distributions make the same kind of patch (Arch Linux and Debian, IIRC).

Thanks again for the review and comments.  I greatly appreciate it.

(In reply to comment #7)
> Hello Jussi!  Thank you for your comments.
> 
> "- If you need to redefine the prefix and mandir then the makefile probably
> doesn't support DESTDIR, which should be fixed in the first place."
> 
> It does support DESTDIR, but sets PREFIX to /usr/local.  Here is a snippet from
> the unpatched makefile:

Right. This program doesn't use autotools and thus the prefix is not set by %configure. OK.

> install:
>         $(INSTALLDIR) $(DESTDIR)$(PREFIX)/bin
>         $(INSTALLBIN) $(PROG) $(DESTDIR)$(PREFIX)/bin/$(PROG)
>         $(INSTALLDIR) $(DESTDIR)$(PREFIX)/man/man1
>         $(INSTALLMAN) $(PROG).1 $(DESTDIR)$(PREFIX)/man/man1/$(PROG).1
> 
> So I think this patch is necessary to insert $MANDIR in the relevant places and
> then PREFIX and MANDIR can be defined at make install.  The spec uses
> DESTDIR=$RPM_BUILD_ROOT too.

Yes, also the man should go to $(PREFIX)/share/man/man1/ instead of $(PREFIX)/man/man1.


> "- You don't need a patch to set the man page permissions, you can just chmod
> it
> after the install. (You can contact upstream about them, though.)"
> 
> That patch also removes some ownership bits as well:
> 
>  PREFIX?= /usr/local
>  INSTALLDIR= install -d
> -INSTALLBIN= install -g bin -o root -m 555
> -INSTALLMAN= install -g bin -o root -m 444
> +INSTALLBIN= install -p
> +INSTALLMAN= install -p -m 644
> 
> Perhaps the patch should remove the ownership/permission bits entirely and like
> you say I can chmod the man page to 644 -- I guess I would do that in %files? 

You should be able to override these with
 make install PREFIX=%{_prefix} MANDIR=%{_mandir} DESTDIR=$RPM_BUILD_ROOT INSTALLBIN="install -p -m 755" INSTALLMAN="install -p -m 644"
instead of patching. But yes, the install command shouldn't specify a user or a group.

> "- What happens if you don't use patch2? Doesn't -I work as well as -iquote..?"
> 
> It fails to build in i386 and PPC.  Some other distributions make the same kind
> of patch (Arch Linux and Debian, IIRC).

Interesting. You saw this with a koji scratch build?

About man going (In reply to comment #8) 
> > install:
> >         $(INSTALLDIR) $(DESTDIR)$(PREFIX)/bin
> >         $(INSTALLBIN) $(PROG) $(DESTDIR)$(PREFIX)/bin/$(PROG)
> >         $(INSTALLDIR) $(DESTDIR)$(PREFIX)/man/man1
> >         $(INSTALLMAN) $(PROG).1 $(DESTDIR)$(PREFIX)/man/man1/$(PROG).1
> > 
> > So I think this patch is necessary to insert $MANDIR in the relevant places and
> > then PREFIX and MANDIR can be defined at make install.  The spec uses
> > DESTDIR=$RPM_BUILD_ROOT too.
> 
> Yes, also the man should go to $(PREFIX)/share/man/man1/ instead of
> $(PREFIX)/man/man1.

Yes, the fixmanpagedir patch does this:

diff -up tmux-0.8/GNUmakefile.fixmanpagedir tmux-0.8/GNUmakefile
--- tmux-0.8/GNUmakefile.fixmanpagedir  2009-06-15 21:30:09.000000000 -0400
+++ tmux-0.8/GNUmakefile        2009-06-15 21:30:47.000000000 -0400
@@ -65,6 +65,7 @@ LDFLAGS+=
 LIBS+= -lncurses

 PREFIX?= /usr/local
+MANDIR?= ${PREFIX}/man
 INSTALLDIR= install -d
 INSTALLBIN= install -p
 INSTALLMAN= install -p -m 644
@@ -137,8 +138,8 @@ depend: $(SRCS)
 install:
        $(INSTALLDIR) $(DESTDIR)$(PREFIX)/bin
        $(INSTALLBIN) $(PROG) $(DESTDIR)$(PREFIX)/bin/$(PROG)
-       $(INSTALLDIR) $(DESTDIR)$(PREFIX)/man/man1
-       $(INSTALLMAN) $(PROG).1 $(DESTDIR)$(PREFIX)/man/man1/$(PROG).1
+       $(INSTALLDIR) $(DESTDIR)$(MANDIR)/man1
+       $(INSTALLMAN) $(PROG).1 $(DESTDIR)$(MANDIR)/man1/$(PROG).1

 clean:
        rm -f $(CLEANFILES)

This does not change the default upstream locations allows passing MANDIR at make install.

> You should be able to override these with
>  make install PREFIX=%{_prefix} MANDIR=%{_mandir} DESTDIR=$RPM_BUILD_ROOT
> INSTALLBIN="install -p -m 755" INSTALLMAN="install -p -m 644"
> instead of patching. But yes, the install command shouldn't specify a user or a
> group.
> 

Okay, I can try that instead.  Thanks!

> > "- What happens if you don't use patch2? Doesn't -I work as well as -iquote..?"
> > 
> > It fails to build in i386 and PPC.  Some other distributions make the same kind
> > of patch (Arch Linux and Debian, IIRC).
> 
> Interesting. You saw this with a koji scratch build?  

Yep:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1416014

Updated Spec and SRPM:

Spec URL: http://chessgriffin.com/files/pkgs/fedora/tmux/tmux.spec
SRPM URL: http://chessgriffin.com/files/pkgs/fedora/tmux/tmux-0.8-3.fc11.src.rpm

I removed the 'fixperms' patch and instead set ownership and permissions at make install stage per Jussi's suggestion.  I also bumped version to 3 and updated the changelog.

I checked rpmlint -i on the spec, RPM, and SRPM and had no warnings or errors. 
I also built in mock with f11-x86_64 and f11-i386 chroot with no errors. 
Finally, I tested this updated release on koji and it appears to build fine:

https://koji.fedoraproject.org/koji/taskinfo?taskID=1428109

Thanks!

The package is not using the Fedora optimization flags (available through %{optflags} or $RPM_OPT_FLAGS). You need to patch the makefile to get these into use. Keep the -D and -I and the -std switches, remove everything else (as the -W switches). You don't need to touch the libs. 

- Add LDFLAGS="$RPM_OPT_FLAGS" to the end of the make command to use the optimization flags in the linking process too (if the object files have been compiled e.g. with -fPIC then linking will fail without it).

- Instead of
 examples/*
I'd ship
 examples/
since this is a bit clearer.

- Add TODO to %doc.


I can sponsor you, if you first show me that you know the guidelines. To do that you need to read the Packaging and the Review Guidelines and to demonstrate you understand them by making at least one another submission and do a couple of informal reviews of other people's packages (you'll be able to do formal ones once I have sponsored you).

(In reply to comment #11)
> The package is not using the Fedora optimization flags (available through
> %{optflags} or $RPM_OPT_FLAGS). You need to patch the makefile to get these
> into use. Keep the -D and -I and the -std switches, remove everything else (as
> the -W switches). You don't need to touch the libs. 

Okay, I have this working here, but want to make sure of something before I post a new spec:  I should remove the entire -ifeq (${CC},gcc)/-endif block that contains all the -W switches?  I have done that and also patched in $(RPM_OPT_FLAGS) to the makefile and it all builds ok here in mock and I can now see that the Fedora optimization flags are being used.  Still, I wanted to make sure of my understanding about the -W flags.

> 
> - Add LDFLAGS="$RPM_OPT_FLAGS" to the end of the make command to use the
> optimization flags in the linking process too (if the object files have been
> compiled e.g. with -fPIC then linking will fail without it).

Done.  I tried this before but it failed, but most likely because I had not patched the makefile as you pointed out.  :-)  Patching the makefile to add in the $(RPM_OPT_FLAGS) works fine now.

> 
> - Instead of
>  examples/*
> I'd ship
>  examples/
> since this is a bit clearer.

Yes, good point.  Thanks.

> 
> - Add TODO to %doc.
> 
>

Done.
 
> I can sponsor you, if you first show me that you know the guidelines. To do
> that you need to read the Packaging and the Review Guidelines and to
> demonstrate you understand them by making at least one another submission and
> do a couple of informal reviews of other people's packages (you'll be able to
> do formal ones once I have sponsored you).  

Jussi, thank you very much.  I will continue to study the Guidelines and demonstrate my understanding in the manner that you suggest.  I appreciate the feedback and willingness to sponsor me.

Cheers!

(In reply to comment #12)
> (In reply to comment #11)
> > The package is not using the Fedora optimization flags (available through
> > %{optflags} or $RPM_OPT_FLAGS). You need to patch the makefile to get these
> > into use. Keep the -D and -I and the -std switches, remove everything else (as
> > the -W switches). You don't need to touch the libs. 
> 
> Okay, I have this working here, but want to make sure of something before I
> post a new spec:  I should remove the entire -ifeq (${CC},gcc)/-endif block
> that contains all the -W switches?  I have done that and also patched in
> $(RPM_OPT_FLAGS) to the makefile and it all builds ok here in mock and I can
> now see that the Fedora optimization flags are being used.  Still, I wanted to
> make sure of my understanding about the -W flags.

Sure, you can do that, since you don't need to take into account other compilers than gcc.

The -W switches control warnings that the compiler displays, so they're mostly a cosmetic change. $(RPM_OPT_FLAGS) has a -Wall, though, so it's better to remove anything that might collide with it.

btw. you can see what rpm macros do with
 $ rpm --eval %{optflags}
(I don't know if it's possible to do the same thing with $RPM_OPT_FLAGS, but these are just two different styles of writing the same thing as %{buildroot} and $RPM_BUILD_ROOT.)

(In reply to comment #13)
> Sure, you can do that, since you don't need to take into account other
> compilers than gcc.
> 
> The -W switches control warnings that the compiler displays, so they're mostly
> a cosmetic change. $(RPM_OPT_FLAGS) has a -Wall, though, so it's better to
> remove anything that might collide with it.

Right, thank you for the clarification.

> 
> btw. you can see what rpm macros do with
>  $ rpm --eval %{optflags}
> (I don't know if it's possible to do the same thing with $RPM_OPT_FLAGS, but
> these are just two different styles of writing the same thing as %{buildroot}
> and $RPM_BUILD_ROOT.)  

Yep, I had checked those out already.  I also made the macro style used consistent in the spec file.  It mostly was, but there were a few places where it was inconsistent.

Updated Spec:
http://chessgriffin.com/files/pkgs/fedora/tmux/tmux.spec

Updated SRPM:
http://chessgriffin.com/files/pkgs/fedora/tmux/tmux-0.8-4.fc11.src.rpm

These have also been put through rpmlint, mock, and koji with no errors or warnings reported.

Thanks!

- Now there's  -iquote. twice in the build flags: it's already added in the INCDIRS variable where you replaced -I- to -iquote. , but you have added another one in the Linux section after -icompat.

- I wouldn't use a patch for the iquote issue, I'd just use a sed oneliner in %setup which does the same thing:
 sed -i "s| -I- | -iquote. |g" GNUMakefile
That or I'd merge the iquote thing with the optflags patch. Anyway, this is up to you. To me unnecessary patch files are just unnecessary trouble if they aren't needed for very long (this should be fixed in the next upstream release, right?).

**

rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. NEEDSWORK
- License is ISC and BSD instead of plain BSD, see http://fedoraproject.org/wiki/Licensing.
* Most of the files are licensed under ISC
* Some of the files in compat/ are under the 2-clause and 3-clause BSD licenses
=> resulting license is ISC and BSD.
(You can use Debian's licensecheck.pl as a first step to audit the licenses.)

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK
- No license file included, you should ask upstream to add one.

SHOULD: The package builds in mock. OK


**

So, apart from the license issue this package is good to go. I won't give you an approval yet, I want to see some more action on your part first.

(In reply to comment #15)
> - Now there's  -iquote. twice in the build flags: it's already added in the
> INCDIRS variable where you replaced -I- to -iquote. , but you have added
> another one in the Linux section after -icompat.
> 
> - I wouldn't use a patch for the iquote issue, I'd just use a sed oneliner in
> %setup which does the same thing:
>  sed -i "s| -I- | -iquote. |g" GNUMakefile
> That or I'd merge the iquote thing with the optflags patch. Anyway, this is up
> to you. To me unnecessary patch files are just unnecessary trouble if they
> aren't needed for very long (this should be fixed in the next upstream release,
> right?).

Done via sed invocation.

> 
> MUST: The License field in the package spec file must match the actual license.
> NEEDSWORK
> - License is ISC and BSD instead of plain BSD, see
> http://fedoraproject.org/wiki/Licensing.
> * Most of the files are licensed under ISC
> * Some of the files in compat/ are under the 2-clause and 3-clause BSD licenses
> => resulting license is ISC and BSD.
> (You can use Debian's licensecheck.pl as a first step to audit the licenses.)

Ah, thanks for the heads up on that nifty tool.  :-)  I have modified the license field.

Updated Spec URL:
http://chessgriffin.com/files/pkgs/fedora/tmux/tmux.spec

Updated SRPM URL:
http://chessgriffin.com/files/pkgs/fedora/tmux/tmux-0.8-5.fc11.src.rpm

Builds cleanly in mock and koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1431851

> 
> 
> **
> 
> So, apart from the license issue this package is good to go. I won't give you
> an approval yet, I want to see some more action on your part first.  

Understood, thanks again for the review and helpful suggestions.  This has been a big help to me in understanding some of the Fedora packaging nuances.

Cheers!

ping, have you done any other submissions or informal package reviews?

*** Bug 510869 has been marked as a duplicate of this bug. ***

just for your information:
0.9 is out

@ Jussi, I've made just a couple of minor comments here and there, but I will be doing more.  Summer has been busy for my kids :-) thanks for the ping.

@ Simon, thanks.  I will work on an update for version 0.9.

Just a quick follow up.  I have an update working and building fine in koji, but I want to do some further testing and review before posting the update.  I will have something to post in the next day or two.

Updated SPEC and SRPM for version 0.9.  Builds and works fine on my F11 box. I get no rpmlint warnings or errors on the spec, SRPM, or RPM.  It builds fine in Koji dist-f11 [1] and dist-f12 [2].

SPEC URL:
http://chessgriffin.com/files/pkgs/fedora/tmux/tmux.spec

SRPM URL:
http://chessgriffin.com/files/pkgs/fedora/tmux/tmux-0.9-1.fc11.src.rpm

@ Jussi:  I also worked on a fix for nautilus-search-tool:
https://bugzilla.redhat.com/show_bug.cgi?id=477810

[1] https://koji.fedoraproject.org/koji/taskinfo?taskID=1477359
[2] https://koji.fedoraproject.org/koji/taskinfo?taskID=1477382

(In reply to comment #22)
> @ Jussi:  I also worked on a fix for nautilus-search-tool:
> https://bugzilla.redhat.com/show_bug.cgi?id=477810

That's great, but isn't really related to package reviews..

ping, any progress on the reviews?

Jussi, I plan to have another submission in the next day.

ping

Thanks for the ping.  I'm on vacation with my wife and kids and only have very limited internet access.  I'll be back home on Monday, August 24 and will try to get back on some informal reviews shortly thereafter.

ping?

This doesn't seem to go anywhere which is a shame as I would really like to see tmux in fedora.

I'm going to close this bug and open a new review request if there is no progress in the next 10 days.

Hello - sorry about the delay.  My work schedule has been overwhelming and I have had little time to hack on things.  Although I would like to maintain this package and perhaps a few others in Fedora, it seems my schedule prevents me from being able to be an active packager and contributor.  I can package stuff, but I just do not have time to do quality reviews.  Anyway, I do not know what the next step is here, but if someone else wants to pick this up or use my spec file as a starting point to resubmit a new review request, that is fine with me.  Again, sorry about the delay.

Thanks Chess. I have created a new review request bug and am closing this one now.

If you have the time to finish the sponsorship process at some point in the future, let me know. I'll happily reassign the tmux ownership back to you - it's your package after all.

*** This bug has been marked as a duplicate of bug 530743 ***

Okay, thanks Sven!  Good luck -- I also like tmux quite a bit (obviously, :-)) so I hope you are able to get this approved and in to Fedora.  Let me know if there's anything I can do to help and I'll try my best.