SPEC: http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend.spec SRPM: http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend-1.1.29-2.el6.src.rpm Description: Generic program files needed by the Spacewalk server machines. This package includes the common code required by all servers/proxies. Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2305031 Beware of a lot rpmlint warnings. I'm going to explain them: spacewalk-backend.noarch: W: log-files-without-logrotate /var/log/rhn it is just directory, all files and logrotate scripts are handled by subpackages spelling-error all are false negatives conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/* we track them as config files, but want them to be replaced during upgrade, no customization should be placed in these files non-conffile-in-etc /etc/rhn/default/* they are not config files, but rather templates for customizing /etc/rhn/rhn.conf for more details see BZ 523631 explicit-lib-dependency python-hashlib explicit-lib-dependency spacewalk-backend-xml-export-libs false negatives cause by lib suffix of required packages
I can review the package
...but first you'll have to actually upload that SRPM :-)
Err sorry, small typo. The files is there, but correct url is: http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend-1.1.29-22.el6.src.rpm
(In reply to comment #0) > SPEC: > http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend.spec > SRPM: > http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend-1.1.29-2.el6.src.rpm > > Description: > Generic program files needed by the Spacewalk server machines. > This package includes the common code required by all servers/proxies. > > Scratch build: > http://koji.fedoraproject.org/koji/taskinfo?taskID=2305031 > > Beware of a lot rpmlint warnings. I'm going to explain them: > > spacewalk-backend.noarch: W: log-files-without-logrotate /var/log/rhn > it is just directory, all files and logrotate scripts are handled by > subpackages No problem > spelling-error > all are false negatives OK > conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/* > we track them as config files, but want them to be replaced during upgrade, no > customization should be placed in these files > > non-conffile-in-etc /etc/rhn/default/* > they are not config files, but rather templates for customizing > /etc/rhn/rhn.conf for more details see BZ 523631 Then I believe these files should go into %{_datadir} not %{_sysconfdir}. FHS seems to agree with me: "The /usr/share hierarchy is for all read-only architecture independent data files." Other packages are using similar scheme, when example configs are stored in /usr/share and actual configuration in /etc. But as you said on IRC this has to be fixed upstream. > explicit-lib-dependency python-hashlib > explicit-lib-dependency spacewalk-backend-xml-export-libs > false negatives cause by lib suffix of required packages No problem On to the official review...
NEEDSWORK: rpmlint must be run on every package. The output should be posted in the review. I am not going to post whole rpmlint output because it's huge with lot of repeated stuff. spacewalk-backend.noarch: W: incoherent-version-in-changelog 1.1.29-2 ['1.1.29-22.fc13', '1.1.29-22'] This can be fixed easily.... Several: spacewalk-backend.noarch: W: manual-page-warning /usr/share/man/man8/spacewalk-cfg-get.8.gz 1: warning: `\"' not defined Not sure how to fix that, but it would be good to actually do it. spacewalk-backend-server.noarch: W: dangerous-command-in-%pre install spacewalk-backend-server.noarch: W: dangerous-command-in-%post rm This is new package, there is no need to do migrate secret key. Please try to get rid of those pre/post install/rm commands. Whole %pre section seems not needed, and also those remove commands on rhnSecret.py If you really have to keep them in, at least put them in ifdefs so that problematic parts won't be run on Fedoras. spacewalk-backend-tools.noarch: W: no-manual-page-for-binary update-packages spacewalk-backend-tools.noarch: W: no-manual-page-for-binary spacewalk-repo-sync spacewalk-backend-tools.noarch: W: no-manual-page-for-binary rhn-entitlement-report spacewalk-backend-tools.noarch: W: no-manual-page-for-binary spacewalk-update-signatures Contact upstream (should be pretty easy :-) ) and provide man pages for these binaries. OK: The package must be named according to the Package Naming Guidelines . OK: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. . OK (problems explained): The package must meet the Packaging Guidelines . OK: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . OK: The License field in the package spec file must match the actual license. OK: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc. OK: All independent sub-packages have License of their own (if it exists) OK: The spec file must be written in American English. OK: The spec file for the package MUST be legible. OK: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. OK: The package MUST successfully compile and build into binary rpms on at least one primary architecture. NEEDSWORK: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. -libs sub-package has BR on python-devel. Better use python2-devel Also use proper macros to define python_sitearch on F12/RHEL-5 Requires: /etc/rhn doesn't seem to work for me: repoquery --enablerepo=rawhide --whatprovides /etc/rhn returns nothing and none of rpms generated by this SRPM provides this directory either. Perhaps I am completely wrong, so please explain. I also don't think Requires(pre) does what you think it does. Usually Requires(pre) is accompanied with Requires:. When you have "Requires(pre): httpd" it means that you need to have httpd during installation phase and its transaction before spacewalk-backend. However by using (pre), you are saying you don't need this package to actually work/run. That means httpd package could be uninstalled after installing spacewalk and no dependency would prevent this. Solution: add also "Requires: httpd", leaving "Requires(pre): httpd" as is. You have several R(pre) uses in the spec file so re-check all of them. OK: Packages must NOT bundle copies of system libraries. OK: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. OK: A Fedora package must not list a file more than once in the spec file's %files listings. OK: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. OK: Each package must consistently use macros. OK: The package must contain code, or permissable content. NA: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). NA: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. OK: Header files must be in a -devel package. NA: Static libraries must be in a -static package. NA: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. NA: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} NA: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built. NA: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. OK (commented): Packages must not own files or directories already owned by other packages. OK: All filenames in rpm packages must be valid UTF-8. Other: * use %global instead of %define (see https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define) * instead of %{_prefix}/share/rhn use %{_datadir}/rhn * to call python use %{__python} macro (in %post server, in case you won't remove it) * I guess this was supposed to be replaced? "XXX To be determined if the proper location is under backend" * Upstream provides one tarball, yet this spec file has 18 sub-packages. Wouldn't it be easier to just create one monolithic package? Or get upstream to split releases into smaller tarballs and create small package for each. I might have more comments later because the spec file is quite big so it's easy to miss things.
Somehow forgot: most of sub-packages install python files into /usr/share. This is not proper way to deal with python packaging. These files should be moved into proper %{python_sitelib} sub-directory
In this comments I list issues which I addressed without any objections. Items which I would like to comment or discuss will be sent in separate comment so we can better back reference. >NEEDSWORK: All build dependencies must be listed in BuildRequires, except for >any that are listed in the exceptions section of the Packaging Guidelines ; >inclusion of those as BuildRequires is optional. Apply common sense. > >-libs sub-package has BR on python-devel. Better use python2-devel > Also use proper macros to define python_sitearch on F12/RHEL-5 addressed ad man pages. filled BZs: 623965, 623967, 623964, 623966 I will address them in near future. > * use %global instead of %define (see addressed > to call python use %{__python} macro addressed >I guess this was supposed to be replaced? "XXX To be determined if the... yes. removed this XXX > Requires(pre) is accompanied with Requires:. When you have addressed >instead of %{_prefix}/share/rhn use %{_datadir}/rhn not needed with new definition >most of sub-packages install python files into /usr/share. This >is not proper way to deal with python packaging. These files should be moved >into proper %{python_sitelib} sub-directory addressed
>spacewalk-backend.noarch: W: manual-page-warning >/usr/share/man/man8/spacewalk-cfg-get.8.gz 1: warning: `\"' not defined >Not sure how to fix that, but it would be good to actually do it. Hmmm, I really do not understood it either. I would like to fix it, but I do not how. :(
>spacewalk-backend-server.noarch: W: dangerous-command-in-%pre install >spacewalk-backend-server.noarch: W: dangerous-command-in-%post rm >This is new package, there is no need to do migrate secret key. Please >try to get rid of those pre/post install/rm commands. ... >If you really have to keep them in, at least put them in ifdefs so >that problematic parts won't be run on Fedoras. Yes. This is new package in Fedora, but this is definitely not new package at all. And if I remove this section, everybody who use Fedora and will upgrade to this package will break his installation.
>Requires: /etc/rhn doesn't seem to work for me: >repoquery --enablerepo=rawhide --whatprovides /etc/rhn returns nothing >and none of rpms generated by this SRPM provides this directory >either. Perhaps I am completely wrong, so please explain. This directory is owned by two packages from upstream. One of them ins Review queue: https://bugzilla.redhat.com/show_bug.cgi?id=491331 I did not realize this has this dependency. So yes, I understood that BZ 491331 need to be approved before this one, but this review will be probably long run anyway :)
(In reply to comment #8) > >spacewalk-backend.noarch: W: manual-page-warning > >/usr/share/man/man8/spacewalk-cfg-get.8.gz 1: warning: `\"' not defined > >Not sure how to fix that, but it would be good to actually do it. > Hmmm, I really do not understood it either. I would like to fix it, but I do > not how. :( If your mangpage is generated from docbook, see bug 639347.
I'm still working on this. My ETA is one month.
No longer blocks 639391 as the F14 package got reverted to a version which doesn't depend on this.
Updated SPEC: http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend.spec Updated SRPM: http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend-1.2.38-1.el6.src.rpm
Ok, re-posting rpmlint output since a lot of changes happened. rpmlint output is now much shorted so I am posting it complete, with my comments at the end. spacewalk-backend.noarch: E: non-standard-dir-perm /var/log/rhn 0770L spacewalk-backend.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn.conf spacewalk-backend.noarch: W: log-files-without-logrotate /var/log/rhn spacewalk-backend-app.noarch: W: spelling-error %description -l en_US rhnpush -> Rhodanus, rhonchus, pushpin spacewalk-backend-app.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-app.conf spacewalk-backend-app.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_app.conf spacewalk-backend-applet.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-applet.conf spacewalk-backend-applet.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_applet.conf spacewalk-backend-config-files.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-config-management.conf spacewalk-backend-config-files.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_config-management.conf spacewalk-backend-config-files-tool.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-config-management-tool.conf spacewalk-backend-config-files-tool.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_config-management-tool.conf spacewalk-backend-iss.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-sat.conf spacewalk-backend-iss-export.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-sat-dump-internal.conf spacewalk-backend-libs.noarch: E: explicit-lib-dependency python-hashlib spacewalk-backend-package-push-server.noarch: W: spelling-error Summary(en_US) rhnpush -> Rhodanus, rhonchus, pushpin spacewalk-backend-package-push-server.noarch: W: spelling-error %description -l en_US rhnpush -> Rhodanus, rhonchus, pushpin spacewalk-backend-package-push-server.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-package-push.conf spacewalk-backend-package-push-server.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_upload_package-push.conf spacewalk-backend-package-push-server.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_upload.conf spacewalk-backend-server.noarch: W: conffile-without-noreplace-flag /etc/httpd/conf.d/zz-spacewalk-server-wsgi.conf spacewalk-backend-server.noarch: W: conffile-without-noreplace-flag /etc/httpd/conf.d/zz-spacewalk-server.conf spacewalk-backend-server.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server.conf spacewalk-backend-server.noarch: W: dangerous-command-in-%pre install spacewalk-backend-server.noarch: W: dangerous-command-in-%post rm spacewalk-backend-tools.noarch: E: explicit-lib-dependency python-hashlib spacewalk-backend-tools.noarch: E: explicit-lib-dependency spacewalk-backend-xml-export-libs spacewalk-backend-tools.noarch: W: conffile-without-noreplace-flag /etc/rhn/default/rhn_server_iss.conf spacewalk-backend-tools.noarch: W: manual-page-warning /usr/share/man/man8/spacewalk-debug.8.gz 1: warning: macro `\"' not defined spacewalk-backend-tools.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_satellite.conf spacewalk-backend-xmlrpc.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-xmlrpc.conf spacewalk-backend-xmlrpc.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_xmlrpc.conf spacewalk-backend-xp.noarch: W: conffile-without-noreplace-flag /etc/rhn/satellite-httpd/conf/rhn/spacewalk-backend-xp.conf spacewalk-backend-xp.noarch: W: non-conffile-in-etc /etc/rhn/default/rhn_server_xp.conf spacewalk-backend.src:308: W: deprecated-grep [u'egrep'] 19 packages and 0 specfiles checked; 4 errors, 31 warnings. non-standard-dir-perm for /var/log/rhn is OK since apache will have to have write permissions there. non-conffile-in-etc and conffile-without-noreplace-flag: for a lot of files. Please recheck every file in /etc if it's supposed to be a config file. If yes then they should have %config macro. I assume files under /etc/rhn/default are still used as base config file used by spacewalk. This should be addressed upstream but as such is not a packaging problem (and files can be kept non-config of course). I'd say special care for files in /etc/httpd/conf.d explicit-lib-dependency: false positives deprecated-grep: please use standard grep with proper switches log-files-without-logrotate: provided in sub-packages so no problem So two things: * just make sure you mark config files as config. Fix these things upstream. Only system config files are in /etc. Defaults can be in /usr/share * Use grep instead of egrep Other: Main package contains empty directory /usr/lib/python2.7/site-packages/spacewalk/wsgi and then populated /usr/share/rhn/wsgi/. I assume this is a mistake? sql subpackage has /usr/share/rhn/server/__init__.py file which is a copy of file in /usr/lib/. This file is needed for some sub-packages to work I guess since some are placing files under /usr/share/rhn/server. Otherwise the dir wouldn't be recognized as python module. Am I correct? In that case please instead of having copy of the file, create a symlink from one to the other. LICENSE files don't have to be included in every subpackage since they depend on each other. For example sql doesn't have to include LICENSE because main package includes it and this cascades down. Re. man pages and rpmlint warning: bug is elsewhere so no problem. All in all, package looks MUCH better now. It would be good if the sub-packages also placed files in python_sitelib but even now it's OK.
>non-conffile-in-etc and conffile-without-noreplace-flag: for a lot of files. >Please recheck every file in /etc if it's supposed to be a config file. If yes >then they should have %config macro. I assume files under /etc/rhn/default are >still used as base config file used by spacewalk. This should be addressed >upstream but as such is not a packaging problem (and files can be kept >non-config of course). I'd say special care for files in /etc/httpd/conf.d All those files are marked as %config. They just do not have (noreplace) attribute. Threre is tree kind of files: 1) /etc/rhn/default/* - this is files, which are configuration files. And change in this files will affect functionality of spacewalk-backend. In fact even small change can make spacewalk-backend in non-functional state. For this reason, people are discouraged to edit this file. And edit /etc/rhn/rhn.conf instead. All values in /etc/rhn/rhn.conf overwrite values in /etc/rhn/default/*. We frequently change files in /etc/rhn/default/* and if user (after yum upgrade) did not review .rpmnew files, Spacewalk can remain in non functional state. Or even worse, in state where it will work, but will behave differently from documentated/expected 2) /etc/rhn/satellite-httpd/conf/rhn/* - this are definition files for apache. This define for several locations different apache handlers. Again this is not intendeed for user to edit. But it is still configuration file. 3) /etc/httpd/conf.d - this is only one, which I'm willing to change. Although this will make upgrades little bit complicated for us.
> It would be good if the sub-packages > also placed files in python_sitelib but even now it's OK. Only leftovers in /usr/share/rhn should be in apache handlers. Which are not python modules and even according the Guidelines web aplication should reside in /usr/share/
> sql subpackage has /usr/share/rhn/server/__init__.py file which is a copy of > file in /usr/lib/. This file is needed for some sub-packages to work I guess > since some are placing files under /usr/share/rhn/server. Otherwise the dir > wouldn't be recognized as python module. Am I correct? In that case please > instead of having copy of the file, create a symlink from one to the other. Correct. IMO this change will make Makefile more complicated and will make maintenance harder, I do no think that saving 730 bytes will justify harder maintenance. > LICENSE files don't have to be included in every subpackage since they depend > on each other. I would rather have it there, if it is not problem. > Use grep instead of egrep addressed > /usr/lib/python2.7/site-packages/spacewalk/wsgi and then populated > /usr/share/rhn/wsgi/. I assume this is a mistake? addressed Updated SPEC: http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend.spec Updated SRPM: http://miroslav.suchy.cz/fedora/spacewalk-backend/spacewalk-backend-1.2.48-1.el6.src.rpm
(In reply to comment #16) > 3) /etc/httpd/conf.d - this is only one, which I'm willing to change. Although > this will make upgrades little bit complicated for us. What do you mean? It's up to the administrator to make sure new version of config file uses correct format and has all needed config options (merging .rpmsave or .rpmnew files as needed depending on use of noreplace). It doesn't change anything for the packager. You just have to decide if you prefer to overwrite custom changes or if you take a risk someone uses out-of-date config files... (In reply to comment #18) > > sql subpackage has /usr/share/rhn/server/__init__.py file which is a copy of > > file in /usr/lib/. This file is needed for some sub-packages to work I guess > > since some are placing files under /usr/share/rhn/server. Otherwise the dir > > wouldn't be recognized as python module. Am I correct? In that case please > > instead of having copy of the file, create a symlink from one to the other. > Correct. IMO this change will make Makefile more complicated and will make > maintenance harder, I do no think that saving 730 bytes will justify harder > maintenance. I assume you meant it will make spec file more complicated? One way or the other. FESCO had to give special exception to duplicating LICENSE files when new guidelines came into play. So non-LICENSE files cannot be duplicated in a rpm without FESCO exception. This __init__.py will have to be symlinked or you'll have to ask for exception with FESCO. Sorry I wasn't clear about this in the original review. (it might be possible I understood the guidelines wrong...but someone would have to convince me :-) ) > > LICENSE files don't have to be included in every subpackage since they depend > > on each other. > I would rather have it there, if it is not problem. I am not entirely sure this is "clean" since you are duplicating files when duplication is not needed/required by guidelines. But I asked on fedora-devel and seems like it would be better not to duplicate these LICENSE files if it's not required, but it's not against the guidelines either...so up to you. > > Use grep instead of egrep > addressed > > > /usr/lib/python2.7/site-packages/spacewalk/wsgi and then populated > > /usr/share/rhn/wsgi/. I assume this is a mistake? > addressed Great So could you please addess that one file copy (even the LICENSEs if you feel like it). After that...I believe I'll be able to approve the package with confidence :-)
regarding license: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines if a subpackage is dependent ... upon a base package ... it is *not necessary* for that subpackage to also include those license texts as %doc. regarding the __init__.py after discussion on IRC we did not find any relevant point in Guidelines, but been able to find several instances of __init__.py with same content, which are not symlinked regarding the conf files there is tons of configuration files and a lot of options are tightly closed to code and it is not in power of regular admin to do the merge of conf and conf.rpmnew. Therefore all configs (but /etc/rhn/rhn.conf) are shipped without (noreplace). This is documented behaviour of Spacewalk.
As we discussed these issues on IRC, I consider them solved/explained. Package is good to go from my POV. Package is APPROVED.
New Package CVS Request ======================= Package Name: spacewalk-backend Short Description: Common programs needed to be installed on the Spacewalk servers/proxies Owners: msuchy Branches: F-13, F-14, EL-5, EL-6 InitialCC:
Git done (by process-git-requests).
spacewalk-backend-1.2.74-2.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/spacewalk-backend-1.2.74-2.fc13
spacewalk-backend-1.2.74-2.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/spacewalk-backend-1.2.74-2.fc14
spacewalk-backend-1.2.74-2.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.
spacewalk-backend-1.2.74-2.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report.