Spec URL: http://miroslav.suchy.cz/fedora/NPusers/NPusers.spec URL: http://miroslav.suchy.cz/fedora/NPusers/NPusers-1.17.11-6.fc9.src.rpm Description: Installs NOCpulse users Note: I still do not have sponsor although I requested it in BZ 452832.
Do not consider it an offense, but this is very poorly pacakged, with minimal effort spent to comply with Fedora quality requirements. A couple of examples: 1.) This is completely pointless, please do not do it Source9999: version Version: %(echo `awk '{ print $1 }' %{SOURCE9999}`) Release: %(echo `awk '{ print $2 }' %{SOURCE9999}`)%{?dist} 2.) Do not use file dependencies, as they are slow, unless you have to. This is not the case here, all these are in well-known packages. Requires: /usr/sbin/useradd /bin/chmod /bin/false /usr/bin/passwd /bin/chown /bin/awk Also, for ones you use it in install-time scriptlets use Requires(pre): and Requires(post): respectively. You also use a couple of more utilities, such as ssh-keygen, which you completely miss a require for. 3.) You do not define %main_source anyhere. FWIW, these lines are completely useless %define build_sub_dir %(echo %{main_source} | sed 's/\.tar\.gz$//') %setup -n %build_sub_dir 4.) Replace absolute paths with macros, such as %{_sysconfdir} and %{_localstatedir}. See rpm --showrc for more. # Install the user creation script install -m 755 -d %buildroot/etc/nocpulse install -m 755 -d %buildroot/var/log/nocpulse ... 5.) Completely incorrect %files list %pre ... # Setting up nocpulse homedir and ssh key pair for dir in /etc/nocpulse /var/lib/nocpulse/{,.ssh,var{,/archives}} do if [ ! -d $dir ] then mkdir -p $dir fi done /usr/bin/ssh-keygen -q -t dsa -N '' -f /var/lib/nocpulse/.ssh/nocpulse-identity chown -R nocpulse.nocpulse /var/lib/nocpulse %files %defattr(-,nocpulse,nocpulse) %dir /etc/nocpulse %dir /var/log/nocpulse Files list refers to the state of the tree at package build time. Files that are created at install- or run-time should use %ghost. 6.) You are obviously not on Solaris in Fedora; do you really need to keep Solaris stuff here? Also this seems to deal with user and group management, see https://fedoraproject.org/wiki/Packaging/Guidelines#Users_and_Groups if [ $OSTYPE = solaris ] ; then SOLARIS=true sysacct= wheel_group=apache oracle_group=dba tcsh=/usr/local/bin/tcsh orac else SOLARIS= sysacct=-r wheel_group="-G apache" oracle_group=oinstall tcsh=/bin/tcsh fi I could proceed on an on; (%build section missing, etc., etc.) but the package is really not ready for any qa, running rpmlint would be a nonsense at this point.
Sorry for not being more constructive, but the package needs to be completely reworked to be acceptable for Fedora, and in its current state can not serve as a basis for any constructive criticism. I recommend you to spend some time getting comfortable with the packaging guidelines: https://fedoraproject.org/wiki/Packaging/Guidelines Also, reading existing Fedora SPEC files may help a lot: http://cvs.fedoraproject.org/viewcvs/rpms When it comes to tooling, you'll definitely appreciate rpmlint and rpmdev-tools. rpmdev-newspec may give you a good starting point for a reworked package. Also, I'll gladly help to resolve problems that may arise during your packaging attempts. NB: This package and bug #453111 should probably be integrated into a single package with nocpulse, unless there are really good reasons against it.
Lubo, I appreciate you comments, just to put you in context. NOCpulse is very old code and I already do a lot of cleaning of it (but I know it is not excuse for not clean it further). And although I'm going in incorporate some of your comment I have to disagree on and explain this points: 1) This is not completely pointless. We (RHN Satellite team) put version to separate file (for every package) so we can easily bump up it using our own build scripts. This way we can avoid of situation when somebody forget it to bump up and we end up with two package with same NEVR but different content. If you are aware of better handling of this I'd thankfully if you share this with me. 3) Ops, correct. I forgot to put this lines to spec: Source2: sources %define main_source %(awk '{ print $2 ; exit}' %{SOURCE2}) Source0: %{main_source} Which again seems pointless. But it is not. We often build development versions of rpm (because we are upstream) and the .tar.gz is named as %(name)-<SHA1ofTree>.tar.gz and only release packages have name %(name)-%(version)-tar.gz I'm unwilling to change it because of [1]. 6) nocpulse runs on various arch, so I either keep it there or make spec for each arch. I'm unwilling to change it because of [1]. [1] Making changes to code (or anything in tar.gz) is easily done using patches, src.rpm, rpm and fedora do not have tools how to track changes to spec file. And because we are upstream we either change this spec and then we have to change our team processes or we leave it in spec file. And about comment #2, NB: This package originaly hold more then what is currently inside and in future can containt more or less. Yes, it is possible to intergrated it itno single packages, but a) historicaly it was separate packages, so it probably should stay separate b) some packages may need only users, but not config files, so it should stay separate. c) fact, that both packages are small is no reason for integration. IMO.
Ok, I discussed with team the version file and we can put it directly into spec. But the sources file, which define name of tar.gz file will be litlle bit pain for us to put into spec. But it is doable. So I have question: Can be name of tar.gz defined in file "sources" and read by spec file using: Source2: sources %define main_source %(awk '{ print $2 ; exit}' %{SOURCE2}) Source0: %{main_source} Or this is blocker and I should definitely remove it?
Well I discussed this issue with team, and we can abandon version a source files. All other suggestion was incorporated into spec. New spec: http://miroslav.suchy.cz/fedora/NPusers/nocpulse-users.spec New src.rpm: http://miroslav.suchy.cz/fedora/NPusers/nocpulse-users-1.17.11-7.src.rpm
Any update on this package? I believe that in #5 I addressed all problem stated in #2.
I'll go ahead and start a review. Numbers 4 and 5 I need to look into more. A few notes FAIL - MUST: rpmlint must be run on every package. The output should be posted in the review. rpmlint is not silent. This isn't show stopping, but could be better. rpmlint nocpulse-users-1.17.11-7.fc8.noarch.rpm nocpulse-users.noarch: W: no-documentation nocpulse-users.noarch: W: non-standard-uid /var/lib/nocpulse/.ssh nocpulse nocpulse-users.noarch: W: non-standard-gid /var/lib/nocpulse/.ssh nocpulse nocpulse-users.noarch: W: hidden-file-or-dir /var/lib/nocpulse/.ssh nocpulse-users.noarch: W: hidden-file-or-dir /var/lib/nocpulse/.ssh nocpulse-users.noarch: W: non-standard-uid /var/log/nocpulse nocpulse nocpulse-users.noarch: W: non-standard-gid /var/log/nocpulse nocpulse nocpulse-users.noarch: W: non-standard-uid /var/lib/nocpulse nocpulse nocpulse-users.noarch: W: non-standard-gid /var/lib/nocpulse nocpulse nocpulse-users.noarch: W: non-standard-uid /etc/nocpulse nocpulse nocpulse-users.noarch: W: non-standard-gid /etc/nocpulse nocpulse nocpulse-users.noarch: W: log-files-without-logrotate /var/log/nocpulse nocpulse-users.noarch: W: obsolete-not-provided NPusers 1 packages and 0 specfiles checked; 0 errors, 13 warnings. 1. Could any documentation be provided for this package? 2. Any time you obsolete something, you should provide it also, that way anything depending on what you obsolete doesn't have to be rebuilt. 3. /var/log/nocpulse log should be incorporated into logrotate. System admins will thank you. 4. ??- MUST: The package must contain code, or permissable content. This is described in detail in the code vs. content section of Packaging Guidelines . >>This really isn't code, it's a user 5. ?? - MUST: The License field in the package spec file must match the actual license. >>There isn't really source. Not sure how that works. 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}, in the format %{name}.spec unless your package has an exemption on Package Naming Guidelines . OK - MUST: The package must meet the Packaging Guidelines . OK - MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . NA - MUST: 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 - MUST: The spec file must be written in American English. OK - MUST: The spec file for the package MUST be legible. If the reviewer is unable to read the spec file, it will be impossible to perform a review. Fedora is not the place for entries into the Obfuscated Code Contest (http://www.ioccc.org/). OK - MUST: 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 - MUST: The package must successfully compile and build into binary rpms on at least one supported architecture. NA - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch needs to have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number should then be placed in a comment, next to the corresponding ExcludeArch line. New packages will not have bugzilla entries during the review process, so they should put this description in the comment until the package is approved, then file the bugzilla entry, and replace the long explanation with the bug number. The bug should be marked as blocking one (or more) of the following bugs to simplify tracking such issues: FE-ExcludeArch-x86 , FE-ExcludeArch-x64 , FE-ExcludeArch-ppc , FE-ExcludeArch-ppc64 OK - MUST: 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. OK - MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden. OK - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. OK - MUST: 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. Refer to the Guidelines for examples. OK - MUST: A package must not contain any duplicate files in the %files listing. OK - MUST: 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 - MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ). OK - MUST: Each package must consistently use macros, as described in the macros section of Packaging Guidelines . OK - MUST: Large documentation files should 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) OK - MUST: 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. NA - MUST: Header files must be in a -devel package. NA - MUST: Static libraries must be in a -static package. NA - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). NA - MUST: 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 - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} NA - MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec. NA - MUST: 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. This is described in detail in the desktop files section of the Packaging Guidelines . 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 - MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. OK - MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ). See Prepping BuildRoot For %install for details. NA - MUST: All filenames in rpm packages must be valid UTF-8. SHOULD Items: OK - SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. NA - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. OK - SHOULD: The reviewer should test that the package builds in mock. See MockTricks for details on how to do this. OK - SHOULD: The package should compile and build into binary rpms on all supported architectures. OK - SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. OK - SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. OK - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. NA - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. OK - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. Please see File Dependencies in the Guidelines for further information.
Created attachment 313810 [details] IRC log Based on discussion on this IRC log with stahnma and dgilmore. I'm going to merge nocpulse-config and nocpulse-users to one package nocpulse-common.
Updated SPEC: http://miroslav.suchy.cz/fedora/nocpulse-common/nocpulse-common.spec Updated SRPM: http://miroslav.suchy.cz/fedora/nocpulse-common/nocpulse-common-2.0.1-2.src.rpm $ rpmlint /home/msuchy/rpmbuild/RPMS/noarch/nocpulse-common-2.0.1-2.noarch.rpm nocpulse-common.noarch: W: non-standard-uid /var/log/nocpulse nocpulse nocpulse-common.noarch: W: non-standard-gid /var/log/nocpulse nocpulse ^--- nocpulse need to write here nocpulse-common.noarch: W: non-standard-uid /var/lib/nocpulse nocpulse nocpulse-common.noarch: W: non-standard-gid /var/lib/nocpulse nocpulse ^--- home dir of nocpulse user nocpulse-common.noarch: W: obsolete-not-provided NPusers nocpulse-common.noarch: W: obsolete-not-provided np-config ^--- previous versions is not compatible with this version. see irc log for reasoning. 1 packages and 0 specfiles checked; 0 errors, 6 warnings.
Any update?
have a look at http://fedoraproject.org/wiki/Packaging/Guidelines#Running_scriptlets_only_in_certain_situations i think you should only run the %pre and %post scriptlets on install rather than checking each time if things are there or not. you should also remove %define doc_dir %{_docdir}/%{name} and mkdir -p $RPM_BUILD_ROOT%{doc_dir} from the spec file. its not needed. you can and should post a tarball on fedorahosted. please do so and use the full url to the upstream source.
Scriplets fixed in %pre, I left %post as is, becouse in previous version the indentiry file have been on other place, so we want to check it during upgrade. docdir removed I disagree about that tarball. See: http://fedoraproject.org/wiki/Packaging/SourceURL#We_are_Upstream We are upstream. And our upstream is src.rpm, not tar.gz Therefore I provide url to our project and in comment explained how to get this src.rpm Updated SPEC: http://miroslav.suchy.cz/fedora/nocpulse-common/nocpulse-common.spec Updated SRPM: http://miroslav.suchy.cz/fedora/nocpulse-common/nocpulse-common-2.0.6-1.src.rpm
http://fedoraproject.org/wiki/Packaging/SourceURL#We_are_Upstream says "There is no public revision control system or publically released tarball for these programs so there is no tarball to list" there is a public revision control system, and im saying there should be public tarballs. this is something that could be useful and useable by more than spacewalk. spacewalk could also end up in other distros. a traball should be provided. otherwise it looks good.
Hmm, I just wonder why package fedora-release (which is exactly same situation, fedora is upstream for that package) do not have direct link for tar.gz file? http://cvs.fedoraproject.org/viewvc/rpms/fedora-release/F-9/fedora-release.spec?revision=1.72&view=markup
Updated SPEC: http://miroslav.suchy.cz/fedora/nocpulse-common/nocpulse-common.spec Updated SRPM: http://miroslav.suchy.cz/fedora/nocpulse-common/nocpulse-common-2.0.8-1.src.rpm
52289f16f8504f954369c6a3f065eec7892d6649 nocpulse-common-2.0.8.tar.gz 52289f16f8504f954369c6a3f065eec7892d6649 /home/dgilmore/rpmbuild/SOURCES/nocpulse-common-2.0.8.tar.gz sha1sum of source matches rpmlint gives nocpulse-common.noarch: W: non-standard-uid /var/log/nocpulse nocpulse nocpulse-common.noarch: W: non-standard-gid /var/log/nocpulse nocpulse nocpulse-common.noarch: W: non-standard-uid /var/lib/nocpulse nocpulse nocpulse-common.noarch: W: non-standard-gid /var/lib/nocpulse nocpulse nocpulse-common.noarch: W: obsolete-not-provided NPusers nocpulse-common.noarch: W: obsolete-not-provided np-config i disagree with not putting provides in but ill accept it without. builds in mock on rawhide. # This src.rpm is cannonical upstream # You can obtain it using this set of commands # git clone git://git.fedorahosted.org/git/spacewalk.git/ # cd monitoring/nocpulse-common # make srpm needs to be removed from the spec. but can be done at import time. Approved
of course that person who approved the package doesnt have the ability to approve packages. I do approve it. with the same conditions
ping, no cvs admin request yet.
New Package CVS Request ======================= Package Name: nocpulse-common Short Description: NOCpulse common Owners: msuchy Branches: devel F-9 F-10 EL-4 EL-5 InitialCC: Cvsextras Commits: yes
CVS Done