Bug 506355

Some notes:

* src.rpm itself
  - All files in src.rpm should have 0644 permission.

* License
  - As far as I checked the whole source code, the license
    tag should be "GPLv2+".

* %description
  - You don't have to repeat the same explanation on -devel
    subpackage which already appears in the %description of
    main package.

* Fedora specific compilation flags
  https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
  - Fedora specific compilation flags are not correctly honored:
--------------------------------------------------------
   217  make[2]: Entering directory `/builddir/build/BUILD/munge-0.5.8/src/libcommon'
   218  if /bin/sh ../../libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I. -I../../config  -I../../src/libmunge   -D_GNU_SOURCE -MT fd.lo -MD -MP -MF ".deps/fd.Tpo" -c -o fd.lo fd.c; \
--------------------------------------------------------
    ( By the way, please check what %configure actually does
      by $ rpm --eval %configure )

* Timestamps
  https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
  - When using "install" or "cp" commands, add "-p" option to keep
    timestamps on installed files.

* Scriptlets
  - Calling /sbin/ldconfig on -devel %post(un) scriptlets is not needed.
  - Use %_var or %_localstatedir instead of directly using "/var"
  - This is not always needed, however would you check if
    "condrestart" is not needed at %postun?
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets

* %files
  - "INSTALL" file is usually for people who want to build and install
    software by themselves and not needed for rpm users.

  - From the contents of create-munge-key, it seems that
    %files should have %ghost %_sysconfdir/%name/%name.key entry.

  - Usually man pages of entry 3 is for the explanation of APIs of
    functions in libraries or so and should be in -devel subpackage.

  - %defattr must be set on -devel subpackage (please use rpmlint)

  - Currently %{_var}/lib/munge has 0711 permission, which is not
    usual, however explicit %attr is not set for this directory.
    If this permission is correct, set %attr on this directory
    explicitly. Otherwise fix the permission on this directory
    at %install.
    The same issue also applies to %{_sysconfdir}/munge, %{_var}/log/munge

* -devel subpackage
  - munge.h does not work as:
--------------------------------------------------------------------
+++++ TEMP.c ++++++++
#include <munge.h>

int foo (munge_enum_t type){
	return munge_enum_is_valid(type, 0);
}
+++++++++++++++++++++++

$ env LANG=C gcc -c TEMP.c 
In file included from TEMP.c:1:
/usr/include/munge.h:39:4: error: #error By linking against libmunge, the derivative
/usr/include/munge.h:40:4: error: #error work becomes licensed under the terms of the
/usr/include/munge.h:41:4: error: #error GNU General Public License. Acknowledge by
/usr/include/munge.h:42:4: error: #error defining the GPL_LICENSED preprocessor macro.
--------------------------------------------------------------------

* service
--------------------------------------------------------------------
[root@localhost ~]# env LANG=C service munge status
[root@localhost ~]# (returns nothing)
--------------------------------------------------------------------

  - Usually this should be something like:
--------------------------------------------------------------------
[root@localhost ~]# service xttpd status
xttpd is stopped
--------------------------------------------------------------------

(In reply to comment #1)

> $ env LANG=C gcc -c TEMP.c 
> In file included from TEMP.c:1:
> /usr/include/munge.h:39:4: error: #error By linking against libmunge, the
> derivative
> /usr/include/munge.h:40:4: error: #error work becomes licensed under the terms
> of the
> /usr/include/munge.h:41:4: error: #error GNU General Public License.
> Acknowledge by
> /usr/include/munge.h:42:4: error: #error defining the GPL_LICENSED preprocessor
> macro.

Hey, can we patch this out?

1) It's terribly passive-aggressive, and I think our code shouldn't do that
2) We don't do this in any other libraries; we assume users can figure out licensing on their own
3) It's not even correct.
a) the licensing only covers distribution. If you don't distribute, it doesn't really matter
b) is that even a legally enforceable 'agreement'?

Could you change the munge.spec summary field to "MUNGE Uid 'N' Gid Emporium"?  MUNGE is a recursive acronym.

Please drop the dont-exit-form-lib.patch.  All it does is comment out the exit() call in the _log_die routine that it supposed to log a message and then die.  The code expects execution to stop once a fatal error message is logged.  Continuing after that will cause unexpected and likely undesired behavior.

As for GPL_LICENSED, it's fine with me if you patch it out.

You don't need the libgcrypt-devel BuildRequires.  MUNGE requires either OpenSSL or libgcrypt -- not both.  You can force the selection with configure's --with-crypto-lib option, but it will use OpenSSL if it finds it.

ping?

First Mamaro and Chris thanks for you comments.

The majority are addressed:

- Correct License to GPLv2+
- Move man5 pages to the devel package.
- Remove +x bit from create-munge-key source.
- Preserve timestamps when installing files.
- ldconfig not needed on -devel package.
- Do a condrestart when upgrading.
- Remove redundant files from docs.
- chmod /var/lib/munge /var/log/munge and /etc/munge to 700.
- Apply patch to not error when GPL_LICENSED is not set.
- Patch service script to print error on if munge.key not present
  on start only and with a better error.
- Remove dont-exit-form-lib.patch. munge is expecting libmunge to
  do this.
- Remove libgcrypt-devel from BuildRequires, uses openssl by
  default anyway.
- Mark the munge.key as a ghost file.

Remaining items

1) I need to check what is going on the compilation flags. I'll get
   to this shortly.

2) Reading the Summary: specification again it is meant to be informative 
   so neither 
   Uid 'N' Gid Emporium
   nor 
   MUNGE is a recursive acronym

  is very good.
 
  For now I've set it to 
  "Enables uid & gid authentication across a host cluster"
  but that is not particularly descriptive either? Will have a think.

Spec and .srm.rpm:
http://cern.ch/steve.traylen/munge-rpms/munge-0.5.8-2.fc11.src.rpm
http://cern.ch/steve.traylen/munge-rpms/munge.spec

$ rpmlint SPECS/munge.spec SRPMS/munge-0.5.8-2.fc11.src.rpm \
        RPMS/x86_64/munge-0.5.8-2.fc11.x86_64.rpm \
        RPMS/x86_64/munge-devel-0.5.8-2.fc11.x86_64.rpm \
        RPMS/x86_64/munge-debuginfo-0.5.8-2.fc11.x86_64.rpm  \
munge.x86_64: W: shared-lib-calls-exit /usr/lib64/libmunge.so.2.0.0
exit.5
munge.x86_64: W: non-standard-uid /var/lib/munge munge
munge.x86_64: W: non-standard-gid /var/lib/munge munge
munge.x86_64: E: non-standard-dir-perm /var/lib/munge 0700
munge.x86_64: W: non-standard-uid /etc/munge/munge.key munge
munge.x86_64: W: non-standard-gid /etc/munge/munge.key munge
munge.x86_64: E: non-readable /etc/munge/munge.key 0400
munge.x86_64: W: non-standard-uid /etc/munge munge
munge.x86_64: W: non-standard-gid /etc/munge munge
munge.x86_64: E: non-standard-dir-perm /etc/munge 0700
munge.x86_64: W: non-standard-uid /var/log/munge munge
munge.x86_64: W: non-standard-gid /var/log/munge munge
munge.x86_64: E: non-standard-dir-perm /var/log/munge 0700
munge.x86_64: W: non-standard-uid /var/run/munge munge
munge.x86_64: W: non-standard-gid /var/run/munge munge
munge.x86_64: W: incoherent-subsys /etc/rc.d/init.d/munge $RH_SUBSYS_BASE
munge-debuginfo.x86_64: E: debuginfo-without-sources
4 packages and 1 specfiles checked; 5 errors, 12 warnings.

They can all be explained I feel as mostly private directories
and files for the munged owned process. The last one of course should be 
fixed up with the compilation corrections.

Builds for fc11, fc12, el4 and el5:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493445
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493450
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493455
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493460

Steve.

Updated packages now with correct compiler flags. From the comments
so far I think this is everything so far.

CFLAGS=%{optflags} -DGNU_SOURCE 

is added NOT for the case of .el4 and .el5 where it compiles with
out -DGNU_SOURCE anyway.

http://cern.ch/steve.traylen/munge-rpms/munge-0.5.8-3.fc11.src.rpm
http://cern.ch/steve.traylen/munge-rpms/munge.spec

http://koji.fedoraproject.org/koji/taskinfo?taskID=1494401
http://koji.fedoraproject.org/koji/taskinfo?taskID=1494406
http://koji.fedoraproject.org/koji/taskinfo?taskID=1494413
http://koji.fedoraproject.org/koji/taskinfo?taskID=1494417


$ rpmlint SPECS/munge.spec SRPMS/munge-0.5.8-3.fc11.src.rpm \
    RPMS/x86_64/munge-0.5.8-3.fc11.x86_64.rpm \
    RPMS/x86_64/munge-debuginfo-0.5.8-3.fc11.x86_64.rpm \
    RPMS/x86_64/munge-devel-0.5.8-3.fc11.x86_64.rpm 

munge.x86_64: W: shared-lib-calls-exit /usr/lib64/libmunge.so.2.0.0 exit.5
munge.x86_64: W: non-standard-uid /var/lib/munge munge
munge.x86_64: W: non-standard-gid /var/lib/munge munge
munge.x86_64: E: non-standard-dir-perm /var/lib/munge 0700
munge.x86_64: W: non-standard-uid /etc/munge/munge.key munge
munge.x86_64: W: non-standard-gid /etc/munge/munge.key munge
munge.x86_64: E: non-readable /etc/munge/munge.key 0400
munge.x86_64: W: non-standard-uid /etc/munge munge
munge.x86_64: W: non-standard-gid /etc/munge munge
munge.x86_64: E: non-standard-dir-perm /etc/munge 0700
munge.x86_64: W: non-standard-uid /var/log/munge munge
munge.x86_64: W: non-standard-gid /var/log/munge munge
munge.x86_64: E: non-standard-dir-perm /var/log/munge 0700
munge.x86_64: W: non-standard-uid /var/run/munge munge
munge.x86_64: W: non-standard-gid /var/run/munge munge
munge.x86_64: W: incoherent-subsys /etc/rc.d/init.d/munge $RH_SUBSYS_BASE
4 packages and 1 specfiles checked; 4 errors, 12 warnings.

As mentioned previously munge is expecting libmunge to exist in this case.
The permissions warnings and errors are private directories and files
that the munge user owns.

The incoherent-subsys warning is false  because there is a variable in
that line.

Steve

Well, for -3:

* Misc issue
  - We now recommend to use %defattr(-,root,root,-)

  - If the permission of files/directories on %files list
    are not 0644 (for file) or 0755 (for directory), 
    I recomment to write explicit permissions as %attr in %files 
    like
----------------------------------------------------
%attr(0700,munge,munge)  %dir %{_var}/run/munge
----------------------------------------------------
    ( even if you change the permissions of these files/directories
       at %install )

  - At %changelog:
----------------------------------------------------
- Move man5 pages to the devel package.
----------------------------------------------------
    Perhaps "man3" pages

Thanks Mamoru again:

http://cern.ch/steve.traylen/munge-rpms/munge-0.5.8-4.fc11.src.rpm
http://cern.ch/steve.traylen/munge-rpms/munge.spec

has the 3 changes from Comment #8.

Steve

Well, okay:

----------------------------------------------------------
  This package (munge) is APPROVED by mtasaka
----------------------------------------------------------

It seems that Dennis is going to sponsor you. When you really
get sponsored, would you notice it on this bugzilla?

Yes I'll notice for sure.
Thanks again.

Now that it seems that you are sponsored, please write
CVS request.

New Package CVS Request
=======================
Package Name: munge
Short Description: Enables uid & gid authentication across a host cluster
Owners: stevetraylen
Branches: F-10 F-11 EL-4 EL-5
InitialCC:

CVS done.

munge-0.5.8-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.fc11

munge-0.5.8-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.fc10

munge-0.5.8-4.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.el5

munge-0.5.8-4.el4 has been submitted as an update for Fedora EPEL 4.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.el4

Submitted now for el4,5 fc10,11.

As I understand this will get to rawhide by itself from the devel build.
Steve.

munge-0.5.8-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

munge-0.5.8-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

munge-0.5.8-4.el4 has been pushed to the Fedora EPEL 4 stable repository.  If problems still persist, please make note of it in this bug report.

munge-0.5.8-4.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

When I download munge from its homepage and run:

  rpmbuild --rebuild munge-0.5.8-1.src.rpm

I get the following rpms:

munge-0.5.8-1.x86_64.rpm
munge-debuginfo-0.5.8-1.x86_64.rpm
munge-devel-0.5.8-1.x86_64.rpm
munge-libs-0.5.8-1.x86_64.rpm


Where is the munge-libs rpm in epel?  This is needed if one wants to leverage this package in building slurm, which is an advanced resource manager.

Hi Chris,

The libs are in the munge package.

$ rpm -pql munge-0.5.8-6.fc11.i586.rpm  |grep lib
/usr/lib/libmunge.so.2
/usr/lib/libmunge.so.2.0.0
/var/lib/munge

Concerning slurm I have meant to get around to building it but have not
yet. Last time I looked I got bogged down deciding which options to
enable but it was a while ago now I looked at it so can't remember the
details.

Anyway I can split up munge into munge and munge-libs for sure if it
makes compatibility with the homepage rpms better but would 
like to know why.

It's not obvious to me why you would only need the libs and not 
the rest of munge.

Also since this is released it makes more sense if you can open a new
bug if that is okay.

Thanks for the comments.

Steve

Steve,

I think Chris Dunlap (cdunlap), who is on the cc list for this bug (and a point of contact for munge), can elaborate on why it builds that way.  Here's what I see in my x86_64 package.

#  rpm -qpl XCXV4.X-MUNGE/munge-libs-0.5.8-1.x86_64.rpm
/usr/lib64/libmunge.so.2
/usr/lib64/libmunge.so.2.0.0

BTW, it was great to see munge in epel and I hope we can resolve this issue!

-Chris

Yes I read Chris and assumed the same Chris of course.

Okay I understand there is a difference that the libs 

/usr/lib64/libmunge.so.2
/usr/lib64/libmunge.so.2.0.0

are in epel:munge
rather than upstream:munge-libs.

but when does a problem actually occur with this?

Do you get an error message or something when building slurm?

Steve

Here's what I see when attempting to build slurm:
---
# rpm -qa | grep munge
munge-0.5.8-6.el5
munge-0.5.8-6.el5
munge-devel-0.5.8-6.el5
munge-devel-0.5.8-6.el5
# rpmbuild -tb slurm-2.0.7.tar.bz2
error: Failed build dependencies:
        munge-libs is needed by slurm-2.0.7-1.x86_64
---

So I think SLURM developers are using how munge builds out of the box.  My fear is that if we start to change scope now with SLURM to build using the epel packages, we start down a hairy path.  It would be nice if we could just match how things build with:
   rpmbuild --rebuild PACKAGENAME

But perhaps there were issues with the munge spec file in the beginning and this is why it was packaged like this?

-cdm

Okay,

So the "real" solution is that slurm should not be requiring munge-libs
at all and should be requiring munge-devel and in: 

epel case)
    this would also pull in munge.
upstream case)
    this would pull in munge-libs

the result being the same.

But of course I can change the epel ones since the upstream ones are well
established and it is not bad packaging or anything, just different and
common technique. In fact there are other advantages since it enables
both i386 and x96_64 libs to be installed on x86_64 bit as well.

Please could you submit a new bug with this request. Essentially
so I have something to close as the package makes its way through
release. 

Steve

OK, I have created:

https://bugzilla.redhat.com/show_bug.cgi?id=530128