Bug 453109 - (nocpulse-common) Review Request: nocpulse-common - Add NOCpulse users and includes common files for NOCpulse.
Review Request: nocpulse-common - Add NOCpulse users and includes common file...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dennis Gilmore
Fedora Extras Quality Assurance
:
Depends On:
Blocks: F-Spacewalk 453111
  Show dependency treegraph
 
Reported: 2008-06-27 07:05 EDT by Miroslav Suchý
Modified: 2009-11-17 08:16 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-12-04 07:23:51 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dennis: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)
IRC log (7.79 KB, text/x-log)
2008-08-08 10:27 EDT, Miroslav Suchý
no flags Details

  None (edit)
Description Miroslav Suchý 2008-06-27 07:05:59 EDT
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.
Comment 1 Lubomir Rintel 2008-06-28 17:34:20 EDT
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.
Comment 2 Lubomir Rintel 2008-06-28 17:40:39 EDT
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.
Comment 3 Miroslav Suchý 2008-06-30 08:28:24 EDT
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.
Comment 4 Miroslav Suchý 2008-06-30 11:30:04 EDT
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?
Comment 5 Miroslav Suchý 2008-07-04 10:31:07 EDT
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
Comment 6 Miroslav Suchý 2008-07-30 10:44:25 EDT
Any update on this package?
I believe that in #5 I addressed all problem stated in #2.
Comment 7 Michael Stahnke 2008-08-05 20:42:12 EDT

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.
Comment 8 Miroslav Suchý 2008-08-08 10:27:19 EDT
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.
Comment 9 Miroslav Suchý 2008-08-12 08:14:44 EDT
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.
Comment 10 Miroslav Suchý 2008-10-14 08:55:41 EDT
Any update?
Comment 11 Dennis Gilmore 2008-10-16 09:43:28 EDT
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.
Comment 12 Miroslav Suchý 2008-10-16 11:05:13 EDT
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
Comment 13 Dennis Gilmore 2008-10-17 02:32:00 EDT
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.
Comment 14 Miroslav Suchý 2008-10-17 03:59:32 EDT
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
Comment 16 Dennis Gilmore 2008-10-24 11:02:39 EDT
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
Comment 17 Dennis Gilmore 2008-10-24 11:04:44 EDT
of course that person who approved the package doesnt have the ability to approve packages.

I do approve it.  with the same conditions
Comment 18 Dennis Gilmore 2008-11-03 10:51:18 EST
ping,  no cvs admin request yet.
Comment 19 Miroslav Suchý 2008-11-03 11:11:21 EST
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
Comment 20 Dennis Gilmore 2008-11-03 13:38:44 EST
CVS Done

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