Spec URL: https://github.com/lsm5/spectrwm-rpm/blob/master/SPECS/spectrwm.spec SRPM URL: https://github.com/lsm5/spectrwm-rpm/blob/master/SRPMS/spectrwm-2.2.0-1.fc18.src.rpm Description: Spectrwm is a small dynamic tiling window manager for X11. It tries to stay out of the way so that valuable screen real estate can be used for much more important stuff. It has sane defaults and does not require one to learn a language to do any configuration. It was written by hackers for hackers and it strives to be small, compact and fast. This is my first package and I need a sponsor. Fedora Account System Username: lsm5 Successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5190735
I am not a sponsor but I will help out with package review. Patches need comments https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment Exclusive arch needs a comment explaining why Provides seems redundant Do list files in %files explicitly and changelog is mandatory run rpmlint on the srpm and binary builds
First off, thanks for reviewing. Patches now supported with comments and links to bug reports ExclusiveArch and Provides gotten rid of All files in %files explicitly mentioned Changelog added $ rpmlint -i RPMS/x86_64/spectrwm-2.2.0-1.fc18.x86_64.rpm spectrwm.x86_64: W: no-soname /usr/lib64/libswmhack.so.0.0 1 packages and 0 specfiles checked; 0 errors, 1 warnings. ** Does this warning need to be taken care of? http://lists.fedoraproject.org/pipermail/devel/2012-April/166110.html mentions it's ok to skip invalid-soname, would it apply to no-soname as well ? $ rpmlint -i SRPMS/spectrwm-2.2.0-1.fc18.src.rpm spectrwm.src: W: %ifarch-applied-patch Patch1: spectrwm-2.2.0-lib64.patch A patch is applied inside an %ifarch block. Patches must be applied on all architectures and may contain necessary configure and/or code patch to be effective only on a given arch. 1 packages and 0 specfiles checked; 0 errors, 1 warnings. ** The lib64 patch makes sure the lib file gets installed to /usr/lib64 for x86_64, so that's why it's applied within %ifarch New successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5193463
UPDATES: Patches reduced. LIBDIR and PREFIX are now handled in spec file itself instead of patches . Spec URL: https://github.com/lsm5/rpmbuild/blob/master/SPECS/spectrwm.spec SRPM URL: https://github.com/lsm5/rpmbuild/blob/master/SRPMS/spectrwm-2.2.0-1.fc18.src.rpm New successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5199103 $ rpmlint SPECS/spectrwm.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. $ rpmlint SRPMS/spectrwm-2.2.0-1.fc18.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint RPMS/x86_64/spectrwm-2.2.0-1.fc18.x86_64.rpm spectrwm.x86_64: W: no-soname /usr/lib64/libswmhack.so.0.0 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
Hi Lokesh. Here are a few areas where I'd like to see changes in this spec file: * You've got a lot of unnecessary space in this spec file. Consider replacing all "double" line breaks with single line breaks, and removing the single empty lines in the top section (e.g. between Summary and License, between Patch0 and URL, between Source0 and BuildRequires). You also do not need newline space between the entries under %files. * Consider using the -b flag when applying patches to append a identifier string. It makes it easier to regenerate diffs in the future: %patch0 -p1 -b .foo * Please move scriptlets to the bottom of the spec file. This is a semantic thing, but I prefer the flow of the spec file to match the order of operations for the package build. They should go below %install, before %files. * You have an empty %doc entry. While there does not appear to be any documentation included with this package (not even a README or a license file, just the man pages), you don't need to have an empty %doc entry in the spec. * Be sure you are incrementing Release every time you make a change to the spec file, and adding a new changelog entry. * There seems to be a .desktop file for spectrwm, not sure if that makes sense to package or not. * The package generates a versioned shared library file, but it doesn't use -Wl,-soname,libspectrwm.so.0 Then, in addition to installing libspectrwm.so.0.0.0, you'll want to create symlinks to libspectrwm.so.0 and libspectrwm.so. The unversioned .so file should go in a -devel subpackage, along with the few .h files it pulls in. The devel subpackage needs to Require: %{name} = %{version}-%{release} * You should be using %{name} and %{version} anywhere it is hardcoded (except for the Name: and Version: fields, of course). Notably, in %files and Source entries. Show me a new spec file, and I should be able to finish off this review quickly (and sponsor you).
Spot, Following should hopefully address most of your comments: Spec URL: https://github.com/lsm5/rpmbuild/blob/master/SPECS/spectrwm.spec SRPM URL: https://github.com/lsm5/rpmbuild/blob/master/SRPMS/spectrwm-2.2.0-2.fc20.src.rpm koji build: https://github.com/lsm5/rpmbuild/blob/master/SRPMS/spectrwm-2.2.0-2.fc20.src.rpm $ rpmlint SRPMS/spectrwm-2.2.0-2.fc20.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint SPECS/spectrwm.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. $ rpmlint RPMS/x86_64/spectrwm-2.2.0-2.fc20.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint RPMS/x86_64/spectrwm-devel-2.2.0-2.fc20.x86_64.rpm spectrwm-devel.x86_64: E: invalid-soname /usr/lib64/libswmhack.so.0.0 swm_hack.so spectrwm-devel.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 1 errors, 1 warnings. Could you please take a look at the patch for -Wl,-soname and let me know if I'm doing it right? https://github.com/lsm5/rpmbuild/blob/master/SOURCES/spectrwm-2.2.0-versioned-shared-lib.patch Also, I'm not sure what to include in spectrwm-devel documentation. Is it ok to leave it blank? spectrwm includes man pages in documentation. Finally, I'm guessing when I say 'yum install spectrwm', I would want libswmhack.so.0.0 and symlinks (in spectrwm-devel) installed as well. But since my spec file says spectrwm-devel requires spectrwm, would I have to say 'yum install spectrwm-devel' in order to get the complete installation? Or should I be changing the subpackage name suitably (libspectrwm perhaps)? Or have I gotten it wrong totally? Thanks,
Whoops ... I guess I made a few errors with the lib files ...will get back to this ... please ignore previous comment
Spot, I overlooked quite a few things in the previous version. Should be fixed now. Please take a look: Spec URL: https://github.com/lsm5/rpmbuild/blob/master/SPECS/spectrwm.spec SRPM URL: https://github.com/lsm5/rpmbuild/blob/master/SRPMS/spectrwm-2.2.0-2.fc20.src.rpm koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5244867 $ rpmlint SRPMS/spectrwm-2.2.0-3.fc20.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint RPMS/x86_64/spectrwm-2.2.0-3.fc20.x86_64.rpm spectrwm.x86_64: E: invalid-soname /usr/lib64/libswmhack.so.0.0 swm_hack.so 1 packages and 0 specfiles checked; 1 errors, 0 warnings. $ rpmlint RPMS/x86_64/spectrwm-devel-2.2.0-3.fc20.x86_64.rpm spectrwm-devel.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. $ rpmlint SPECS/spectrwm.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. Could you checkout the patch for -Wl,-soname and let me know if I'm doing it right? https://github.com/lsm5/rpmbuild/blob/master/SOURCES/spectrwm-2.2.0-versioned-shared-lib.patch Also, I'm not sure what to include in spectrwm-devel documentation. Is it ok to leave it blank? spectrwm now includes man pages in documentation. The .h files used by the lib file get installed via the dependencies itself. The only .h file in spectrwm itself is version.h which doesn't look to be installed on other distros, so I'm guessing it isn't really needed. Let me know if there's something I'm missing here. The upstream Makefile doesn't worry about the spectrwm.desktop file. Looks like they leave it to the user to take care of it (asked this on their IRC too, and nobody cared much about this file). I'm able to use spectrwm without it, so I think it's safe to ignore it (unless another user would want it). Thanks,
Correction: New SRPM URL: https://github.com/lsm5/rpmbuild/blob/master/SRPMS/spectrwm-2.2.0-3.fc20.src.rpm
That patch isn't correct. You want the line to look like this: $(CC) -Wl,-soname,libswmhack.so.0 -shared -fpic -o libswmhack.so.$(LVERS) swm_hack.so $(LDADD) You might not want to hardcode it like that, since it uses a variable for the full versioning, but you might need to create a new makefile variable that just has the major soversion. You can leave the -devel package without docs, thats fine. Also, if none of the .h files are useful, you can ignore them. (Same for the .desktop file).
Spot, Thanks. Please checkout changes. The spec and patch URL are the same as in Comment 8. New SRPM URL: https://github.com/lsm5/rpmbuild/blob/master/SRPMS/spectrwm-2.2.0-4.fc20.src.rpm Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5248876 $ rpmlint RPMS/x86_64/spectrwm-2.2.0-4.fc20.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint RPMS/x86_64/spectrwm-devel-2.2.0-4.fc20.x86_64.rpm spectrwm-devel.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. $ rpmlint SRPMS/spectrwm-2.2.0-4.fc20.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint SPECS/spectrwm.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. Since the version numbers aren't hardcoded in the Makefile, do I need to do something similar in the .spec file as well?
You can, if you wish, define a %global macro (or macros) for the sharedlib versioning where you make the symlinks, or, if you're feeling brave, you can patch up the Makefile to make the appropriate symlinks (that's really the more appropriate option and eliminates the chance for a disconnect).
I'll make that change by tonight and update this. Is there anything else I need to consider?
Created attachment 736096 [details] installs a config file to /etc and removes scrotwm symlink
Created attachment 736097 [details] -Wl,-soname for libswmhack.so.0
Created attachment 736098 [details] shlib symlinks handled in Makefile
Alright, new spec file: https://github.com/lsm5/rpmbuild/blob/master/SPECS/spectrwm.spec koji url: http://koji.fedoraproject.org/koji/taskinfo?taskID=5256488 rpmlint outputs are same as in Comment 11. Avoiding hardcoded versions for shlibs meant I had to write shlibs in %files like so: %{_libdir}/libswmhack.so.* Is this valid? I guess it would go against Rahul's comment to explicitly mention all files. All patches are now attached.
Spot, I had another concern: spectrwm's default configuration uses xterm as terminal emulator, xlockmore for screen locking and dmenu for application launching. These aren't required for installing or even running spectrwm, but it would throw some error messages on the screen if these programs weren't found (unless the user has a separate config file beforehand). I feel this would be annoying to new users, so I have included xterm, xlockmore and dmenu in BuildRequires. Is this ok, or should I remove them from BuildRequires?
They should be Requires, not BuildRequires, and they're fine as Requires. A robust installation makes more sense here as the default. As to the %files entry, if you want to entirely avoid hardcoding in the spec, you do need to wildcard as you've described. This is where there is a difference of opinion. Some packagers prefer to hardcode the full name of the shared library files, with versioning (only under %files) so that if the sover changes upstream, your package will fail to build, and you'll have to manually change it. I don't personally think this is necessary, but I leave it to your discretion.
Cool. Thanks for addressing this. Please see the updated SPEC file: https://github.com/lsm5/rpmbuild/blob/master/SPECS/spectrwm.spec
Not sure why I didn't notice this before, but why are you conditionalizing like this: %ifarch x86_64 make -C linux/ %{?_smp_mflags} PREFIX=/usr LIBDIR=%{_libdir} %else make -C linux/ %{?_smp_mflags} PREFIX=/usr %endif The value of LIBDIR=%{_libdir} should be correct for all arches, you can simplify things here by dropping the conditionalization and using LIBDIR=%{_libdir} in all cases. (same for the identical make install case).
Ohh yes, this has now been fixed. That was the first thing that worked for me when I was having trouble with lib64, so I left it at that. The spec URL is the same as in Comment 20.
Last things (I promise), you don't need to run ldconfig on %post/%postun for devel, only for the main package. You also don't need the empty %doc. Everything else looks good. Make those changes and I'll approve this package.
By not needing empty %doc, do you mean, I include the 1st doc file on the same line as %doc? Which is what I have done. %post and %postun for devel removed. Spec URL same as before.
%doc is not a section, it is a per file flag. You don't need to flag manpages as %doc, because RPM already assumes anything under /usr/share/man is %doc. So in your spec, because there are no files that you need to flag as %doc, you can remove it entirely. In a more "normal" package that included things like "README" and "LICENSE" in the source, but doesn't install them during make install, you can have a %files section like this: %files %doc README LICENSE %{_bindir}/omgwtfbbq ... The %doc macro will then magically: A) make %{buildroot}%{_shareddocdir}/%{name}-%{version}/ B) copy any specified files into that directory for you. You can also do: %doc /full/path/to/file/in/a/weird/non/doc/place/but/is/really/documentation and it will simply mark that file as being a %doc file, but this case is rare.
ohh ya, sorry, I see you mentioned it in your very 1st comment, and I guess I read it wrong. Anyway, spec updated: https://github.com/lsm5/rpmbuild/blob/master/SPECS/spectrwm.spec
== Review == Good: - rpmlint checks return: spectrwm-devel.x86_64: W: no-documentation - package meets naming guidelines - package meets packaging guidelines - license (ISC) OK, matches source - spec file legible, in am. english - source matches upstream (9fc4100530005b6d97c1b2fe81e78d0bf10425ecd57f2c523db6b36e83dab103) - package compiles on f18 (x86_64) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates - no duplicate files - permissions ok - macro use consistent - code, not content - no need for -docs - no need for .desktop file - devel package ok - no .la files - post/postun ldconfig ok - devel requires base package n-v-r APPROVED. I'm going to sponsor you right now. You'll be at this step: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored Let me know if you have any questions about how to move forward.
New Package SCM Request ======================= Package Name: spectrwm Short Description: Minimalist tiling window manager written in C Owners: lsm5 Branches: f18 f19 el6 InitialCC:
Email address lsm5 is not a valid bugzilla email address. Either make a bugzilla account with that email address or change your email address in the Fedora Account System https://admin.fedoraproject.org/accounts/ to a valid bugzilla email address and try again.
Jon, I changed my bugzilla email to lsm5 . When I go to my preferences, I don't see the option to change it back to my @buffalo.edu address. It says 'Confirmed email address: lsm5' and isn't editable. Now, if I change my email address in FAS with my @fedoraproject.org , I guess it will keep forwarding emails to itself (??) Please let me know what you think is the best solution. Thanks,
New Package SCM Request ======================= Package Name: spectrwm Short Description: Minimalist tiling window manager written in C Owners: lsm5 Branches: f18 f19 el6 InitialCC: ----- I restored my original buffalo.edu from the duplicate email change confirmation links. The fedora-cvs request is resubmitted. So, bottomline being: never to use my @fedoraproject.org address within Red Hat's Bugzilla or FAS ...is that right? Thanks,
-- I'm including these to try out fedora-review -- Spec url: http://lsm5.fedorapeople.org/rpmbuild/SPECS/spectrwm.spec SRPM url: http://lsm5.fedorapeople.org/rpmbuild/SRPMS/spectrwm-2.2.0-9.fc20.src.rpm
Git done (by process-git-requests).
spectrwm-2.2.0-9.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/spectrwm-2.2.0-9.fc18
spectrwm-2.2.0-9.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/spectrwm-2.2.0-9.fc19
spectrwm-2.2.0-9.fc19 has been pushed to the Fedora 19 testing repository.
spectrwm-2.2.0-9.fc19 has been pushed to the Fedora 19 stable repository.
spectrwm-2.2.0-9.fc18 has been pushed to the Fedora 18 stable repository.
> Then, in addition to installing libspectrwm.so.0.0.0, you'll want to create > symlinks to libspectrwm.so.0 and libspectrwm.so. The unversioned .so file > should go in a -devel subpackage, along with the few .h files it pulls in. > The devel subpackage needs to Require: %{name} = %{version}-%{release} This is completely wrong for this library. :-( See bug 1095967 comment 5