Spec URL: https://raphgro.fedorapeople.org/review/locker/light-locker-settings.spec SRPM URL: https://raphgro.fedorapeople.org/review/locker/light-locker-settings-1.5.0-1.fc22.src.rpm Description: Just a simple settings dialog for light-locker Fedora Account System Username: raphgro
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=9854458
Upstream has fixed the desktop file. https://github.com/the-cavalry/light-locker/issues/57
This is an unofficial review. At a first glance, I see the following errors: > License: GPLv3+ Reading the source and the project URL, it's only GPLv3. > %setup -q You might prefer %autosetup unless you target EPEL<7. See https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#.25autosetup > BuildRequires: python3 Should be python3-devel. See https://fedoraproject.org/wiki/Packaging:Python#BuildRequires > # configure macro does not work IMHO A little explanation would prevent future developers to lose time wondering why. > * Tue May 26 2015 Raphael Groner <projects.rg> > - Please fill this, also with the version stuff. You might want to see the rpmlint errors too: Checking: light-locker-settings-1.5.0-1.fc21.x86_64.rpm light-locker-settings-1.5.0-1.fc21.src.rpm light-locker-settings.x86_64: W: spelling-error %description -l en_US screensaver -> screen saver, screen-saver, screens aver light-locker-settings.x86_64: W: no-version-in-last-changelog light-locker-settings.x86_64: E: no-binary light-locker-settings.x86_64: E: script-without-shebang /usr/share/light-locker-settings/light-locker-settings/light-locker-settings.glade light-locker-settings.x86_64: W: dangling-symlink /usr/share/light-locker-settings/locale /usr/share/locale light-locker-settings.x86_64: W: spurious-executable-perm /usr/share/doc/light-locker-settings/COPYING light-locker-settings.x86_64: W: spurious-executable-perm /usr/share/doc/light-locker-settings/INSTALL light-locker-settings.x86_64: W: no-manual-page-for-binary light-locker-settings light-locker-settings.x86_64: W: install-file-in-docs /usr/share/doc/light-locker-settings/INSTALL light-locker-settings.src: W: spelling-error %description -l en_US screensaver -> screen saver, screen-saver, screens aver light-locker-settings.src: W: no-version-in-last-changelog light-locker-settings.src:36: W: configure-without-libdir-spec 2 packages and 0 specfiles checked; 2 errors, 10 warnings. Also, you might want to create appdata files, or at least notify upstream a bug about it. See https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#AppData_files That's it for now.
(In reply to Yajo from comment #3) Thanks for your comment. Will fix the issues ASAP. Sorry for the delay.
Spec URL: https://raphgro.fedorapeople.org/review/locker/light-locker-settings.spec SRPM URL: https://raphgro.fedorapeople.org/review/locker/light-locker-settings-1.5.0-2.fc22.src.rpm Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=10426299 * Tue Jul 21 2015 Raphael Groner <> - 1.5.0-2 - include python3 patch - include usability patch (upstream bug 1463476) - fix execution bits - fix installation of documentation (In reply to Yajo from comment #3) > This is an unofficial review. > > At a first glance, I see the following errors: > > > License: GPLv3+ > Reading the source and the project URL, it's only GPLv3. From where do you get that it is GPLv3 only? COPYING file says it's GPLv3+ and that any further version of the GPL license is also possible, I don't see anything to forbid that. > > %setup -q > You might prefer %autosetup unless you target EPEL<7. See > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#. > 25autosetup My understanding is that both are possible as alternatives. I am not convinced about applying all patches automatically cause they need proper formatting then, at least for the pathes and you won't be able to use individual patch -p options. > > BuildRequires: python3 > > Should be python3-devel. See > https://fedoraproject.org/wiki/Packaging:Python#BuildRequires Fixed. Found also a python3 patch in upstream tracker. > > # configure macro does not work > > IMHO A little explanation would prevent future developers to lose time > wondering why. Done. > > * Tue May 26 2015 Raphael Groner <projects.rg> > > - > > Please fill this, also with the version stuff. Fixed. > You might want to see the rpmlint errors too: > > Checking: light-locker-settings-1.5.0-1.fc21.x86_64.rpm > light-locker-settings-1.5.0-1.fc21.src.rpm > light-locker-settings.x86_64: W: spelling-error %description -l en_US > screensaver -> screen saver, screen-saver, screens aver Fixed. > light-locker-settings.x86_64: W: no-version-in-last-changelog Fixed, see above. > light-locker-settings.x86_64: E: no-binary ?? > light-locker-settings.x86_64: E: script-without-shebang > /usr/share/light-locker-settings/light-locker-settings/light-locker-settings. > glade Propably due to wrongly set execution bits. Ignore for now. https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions > light-locker-settings.x86_64: W: dangling-symlink > /usr/share/light-locker-settings/locale /usr/share/locale Wrong path according to FHS. Not sure if patchable in upstream Makefile: cp -rf locale $(DESTDIR)/$(PREFIX)/share ln -sf $(PREFIX)/share/locale $(DESTDIR)/$(PREFIX)/share/$(APPNAME)/locale > light-locker-settings.x86_64: W: spurious-executable-perm > /usr/share/doc/light-locker-settings/COPYING > light-locker-settings.x86_64: W: spurious-executable-perm > /usr/share/doc/light-locker-settings/INSTALL Propably due to wrongly set execution bits. Ignore for now. https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions > light-locker-settings.x86_64: W: no-manual-page-for-binary > light-locker-settings Not sure if we've to provide a manpage. > light-locker-settings.x86_64: W: install-file-in-docs > /usr/share/doc/light-locker-settings/INSTALL Fixed. > light-locker-settings.src: W: spelling-error %description -l en_US > screensaver -> screen saver, screen-saver, screens aver Fixed. > light-locker-settings.src: W: no-version-in-last-changelog Fixed, see above. > light-locker-settings.src:36: W: configure-without-libdir-spec > 2 packages and 0 specfiles checked; 2 errors, 10 warnings. > Also, you might want to create appdata files, or at least notify upstream a > bug about it. See > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/ > Guidelines#AppData_files This is no MUST but clearly SHOULD. I'll follow upstream features. https://fedoraproject.org/wiki/Packaging:AppData
Any update here? Can I take the review?
Feel free to do the review. Thanks for your interest!
No SCM request found.
William, if you want to do the official review, please set the fedora‑review flag to ?, I guess you confused with fedora-cvs.
Will review in 3-5 hours.
-> desktop-file-validate %{buildroot}%{_datadir}/applications/%{name}.desktop move to %check section
Some notes about licensing. Python files marked as GPLv3 only (without possible to next version). COPYING file yes, about GPLv3+. Please notice upstream about this crap. -> sed '/INSTALL/d' Makefile.in.in You want to also add "-i" option -> light-locker-settings.x86_64: E: no-binary I'm not happy with custom sh script in /usr/bin which runs "python /usr/share". I would happy to see if it will python script which does "import light-locker-settings; light-locker-settings.run()" or something like this. I hope you will poke upstream. -> light-locker-settings.x86_64: W: dangling-symlink /usr/share/light-locker-settings/locale /usr/share/locale It should be fixed. gettext in light-locker-settings should use system path instead of making such links. -> light-locker-settings.x86_64: W: spurious-executable-perm /usr/share/doc/light-locker-settings/INSTALL -> light-locker-settings.x86_64: W: spurious-executable-perm /usr/share/doc/light-locker-settings/NEWS -> light-locker-settings.x86_64: W: spurious-executable-perm /usr/share/doc/light-locker-settings/AUTHORS -> light-locker-settings.x86_64: W: spurious-executable-perm /usr/share/doc/light-locker-settings/README -> light-locker-settings.x86_64: W: spurious-executable-perm /usr/share/doc/light-locker-settings/COPYING Do "chmod -x" for those files -> light-locker-settings.x86_64: E: script-without-shebang /usr/share/light-locker-settings/light-locker-settings/light-locker-settings.glade Do "chmod -x" for it -> light-locker-settings.x86_64: W: no-manual-page-for-binary light-locker-settings Ignore -> light-locker-settings.x86_64: W: install-file-in-docs /usr/share/doc/light-locker-settings/INSTALL Not needed All stuff about docks SHOULD be installed by `%doc`, not by program. Would be great to make a patch and sent upstream.
(In reply to Igor Gnatenko from comment #12) … > -> light-locker-settings.x86_64: E: no-binary > I'm not happy with custom sh script in /usr/bin which runs > "python /usr/share". I would happy to see if it will python script which > does > "import light-locker-settings; light-locker-settings.run()" or something > like > this. I hope you will poke upstream. Dashes in python module names are not allowed. See PEP 0008. https://www.python.org/dev/peps/pep-0008/
Damn! I detected that runtime needs pygtk2 to work. But pygtk2 is available for python2 only, so no python3 support.
Hello! As always, sorry for delays. About the licensing stuff, I'm sure you are confused when you read in COPYING the line 639 [1]. Notice that line is after "END OF TERMS AND CONDITIONS", and inside "How to Apply These Terms to Your New Programs". Please read condition 14 [2] and see that those are just instructions, but it's up to the developer to choose that clause. Reading the source files, you can see that there's no clause about "later versions", so it's licensed only in GPLv3. That license is also the one that appears in the website. [1]: http://bazaar.launchpad.net/~light-locker-settings-team/light-locker-settings/trunk/view/head:/COPYING#L639 [2]: http://bazaar.launchpad.net/~light-locker-settings-team/light-locker-settings/trunk/view/head:/COPYING#L570
(In reply to Raphael Groner from comment #5) > > light-locker-settings.x86_64: E: no-binary > > ?? https://fedoraproject.org/wiki/Common_Rpmlint_issues#no-binary > > light-locker-settings.x86_64: W: no-manual-page-for-binary > > light-locker-settings > > Not sure if we've to provide a manpage. It's not required AFAIK (W = Warning).
Using /usr/share for .py{,c} files is definitely wrong cause they're not considered as (noarch) data files but library parts. See https://bugs.python.org/issue588756
Upstream writes: "you can change the settings by hand and xfce4-power-manager supports light-locker's settings and there's also gsettings-editor" So, I'll close this review. Thanks for your time and sorry, I did not know about dead upstream till now.