Bug 946856 - Review Request: spectrwm - Minimalist tiling window manager written in C
Summary: Review Request: spectrwm - Minimalist tiling window manager written in C
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Tom "spot" Callaway
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1095967
TreeView+ depends on / blocked
 
Reported: 2013-03-31 11:51 UTC by Lokesh Mandvekar
Modified: 2014-07-01 22:59 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-05-06 04:25:32 UTC
Type: ---
Embargoed:
tcallawa: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
installs a config file to /etc and removes scrotwm symlink (1.06 KB, patch)
2013-04-15 23:27 UTC, Lokesh Mandvekar
no flags Details | Diff
-Wl,-soname for libswmhack.so.0 (701 bytes, patch)
2013-04-15 23:30 UTC, Lokesh Mandvekar
no flags Details | Diff
shlib symlinks handled in Makefile (664 bytes, application/octet-stream)
2013-04-15 23:31 UTC, Lokesh Mandvekar
no flags Details

Description Lokesh Mandvekar 2013-03-31 11:51:01 UTC
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

Comment 1 Rahul Sundaram 2013-03-31 23:59:03 UTC
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

Comment 2 Lokesh Mandvekar 2013-04-01 08:49:39 UTC
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

Comment 3 Lokesh Mandvekar 2013-04-02 16:34:52 UTC
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.

Comment 4 Lokesh Mandvekar 2013-04-02 16:37:34 UTC
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.

Comment 5 Tom "spot" Callaway 2013-04-10 16:02:38 UTC
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).

Comment 6 Lokesh Mandvekar 2013-04-11 15:35:15 UTC
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,

Comment 7 Lokesh Mandvekar 2013-04-11 16:41:57 UTC
Whoops ... I guess I made a few errors with the lib files ...will get back to this ... please ignore previous comment

Comment 8 Lokesh Mandvekar 2013-04-12 10:34:58 UTC
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,

Comment 9 Lokesh Mandvekar 2013-04-12 10:37:56 UTC
Correction: New SRPM URL: https://github.com/lsm5/rpmbuild/blob/master/SRPMS/spectrwm-2.2.0-3.fc20.src.rpm

Comment 10 Tom "spot" Callaway 2013-04-12 16:04:10 UTC
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).

Comment 11 Lokesh Mandvekar 2013-04-13 20:52:08 UTC
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?

Comment 12 Tom "spot" Callaway 2013-04-15 13:55:54 UTC
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).

Comment 13 Lokesh Mandvekar 2013-04-15 16:21:18 UTC
I'll make that change by tonight and update this. Is there anything else I need to consider?

Comment 14 Lokesh Mandvekar 2013-04-15 23:27:37 UTC
Created attachment 736096 [details]
installs a config file to /etc and removes scrotwm symlink

Comment 15 Lokesh Mandvekar 2013-04-15 23:30:30 UTC
Created attachment 736097 [details]
-Wl,-soname for libswmhack.so.0

Comment 16 Lokesh Mandvekar 2013-04-15 23:31:34 UTC
Created attachment 736098 [details]
shlib symlinks handled in Makefile

Comment 17 Lokesh Mandvekar 2013-04-15 23:36:43 UTC
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.

Comment 18 Lokesh Mandvekar 2013-04-16 15:23:52 UTC
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?

Comment 19 Tom "spot" Callaway 2013-04-16 15:29:13 UTC
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.

Comment 20 Lokesh Mandvekar 2013-04-16 15:53:57 UTC
Cool. Thanks for addressing this.

Please see the updated SPEC file: https://github.com/lsm5/rpmbuild/blob/master/SPECS/spectrwm.spec

Comment 21 Tom "spot" Callaway 2013-04-16 16:01:20 UTC
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).

Comment 22 Lokesh Mandvekar 2013-04-16 16:12:24 UTC
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.

Comment 23 Tom "spot" Callaway 2013-04-16 16:18:57 UTC
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.

Comment 24 Lokesh Mandvekar 2013-04-16 16:28:58 UTC
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.

Comment 25 Tom "spot" Callaway 2013-04-16 16:34:03 UTC
%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.

Comment 26 Lokesh Mandvekar 2013-04-16 16:43:27 UTC
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

Comment 27 Tom "spot" Callaway 2013-04-17 16:39:59 UTC
== 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.

Comment 28 Lokesh Mandvekar 2013-04-17 19:47:17 UTC
New Package SCM Request
=======================
Package Name: spectrwm
Short Description: Minimalist tiling window manager written in C
Owners: lsm5
Branches: f18 f19 el6
InitialCC:

Comment 29 Gwyn Ciesla 2013-04-18 14:18:56 UTC
 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.

Comment 30 Lokesh Mandvekar 2013-04-18 14:37:33 UTC
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,

Comment 31 Lokesh Mandvekar 2013-04-18 15:55:41 UTC
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,

Comment 32 Lokesh Mandvekar 2013-04-19 10:10:44 UTC
-- 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

Comment 33 Gwyn Ciesla 2013-04-22 13:25:45 UTC
Git done (by process-git-requests).

Comment 34 Fedora Update System 2013-04-22 15:06:16 UTC
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

Comment 35 Fedora Update System 2013-04-22 15:07:25 UTC
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

Comment 36 Fedora Update System 2013-04-22 17:22:16 UTC
spectrwm-2.2.0-9.fc19 has been pushed to the Fedora 19 testing repository.

Comment 37 Fedora Update System 2013-05-06 04:25:34 UTC
spectrwm-2.2.0-9.fc19 has been pushed to the Fedora 19 stable repository.

Comment 38 Fedora Update System 2013-05-07 18:23:34 UTC
spectrwm-2.2.0-9.fc18 has been pushed to the Fedora 18 stable repository.

Comment 39 Michael Schwendt 2014-06-24 10:31:53 UTC
> 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


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