[Bug 749291] Review Request: dpm-xrootd - xroot interface to the Disk Pool Manager (DPM)

Thu Nov 17 18:14:36 UTC 2011

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

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

--- Comment #5 from Ricardo Rocha <rocha.porto at gmail.com> 2011-11-17 13:14:35 EST ---
Thanks Steve.

New spec and src rpms:
Spec URL: http://rocha.web.cern.ch/rocha/fedora/dpm-xrootd.spec
SRPM URL: http://rocha.web.cern.ch/rocha/fedora/dpm-xrootd-2.2.3-1.src.rpm

I've fixed the issues you mentioned, but also did a main change upstream. I got
tired of loosing time with autotools and changed the build to use cmake
instead.

There's one issue left, regarding the license. I checked the xrootd packages
already in Fedora, and it's defined as BSD + LGPLv2+. I would say we should
keep the same licensing (David agrees)?

For details on the individual fixes, you can have a look inline.

Koji builds (successful):
https://koji.fedoraproject.org/koji/taskinfo?taskID=3522044 (dist-5E-epel)
https://koji.fedoraproject.org/koji/taskinfo?taskID=3522052 (dist-6E-epel)
https://koji.fedoraproject.org/koji/taskinfo?taskID=3522058 (f16)

(In reply to comment #4)
> Here is the review:
> https://bugzilla.redhat.com/show_bug.cgi?id=749291
> 
>  +:ok, =:needs attention, -:needs fixing
> 
> MUST Items:
> [-] MUST: rpmlint must be run on every package.
> rpmlint ./dpm-xrootd.spec 
> ./dpm-xrootd.spec: W: invalid-url Source0: dpm-xrootd-2.2.2.tar.gz
> 
> dpm-xrootd.src: W: invalid-url Source0: dpm-xrootd-2.2.2.tar.gz
> dpm-xrootd.x86_64: W: spelling-error %description -l en_US plugin -> plug in,
> plug-in, plugging
> dpm-xrootd.x86_64: W: spelling-error %description -l en_US gridFTP -> grid Ftp,
> grid-ftp, griddle
> dpm-xrootd.x86_64: W: spelling-error %description -l en_US scalable -> salable,
> scalawag, scalar
> 
> plugin should be plug-in.
> scalable seems not to be word... You can swap "high performance, scalable fault
> tolerant"
> for "high performance, easy to scale and fault tolerant" or something.

Fixed as suggested.

> dpm-xrootd.x86_64: W: shared-lib-calls-exit /usr/lib64/libXrdDPMXmi.so.0.0.0
> _exit at GLIBC_2.2.5
> dpm-xrootd.x86_64: W: shared-lib-calls-exit
> /usr/lib64/libXrdDPMOfsAndN2N.so.0.0.0 _exit at GLIBC_2.2.5
> You commented on this, hopefully fix one day.

Bug added upstream:
https://svnweb.cern.ch/trac/lcgdm/ticket/345

> dpm-xrootd.x86_64: W: dangling-symlink /usr/bin/dpm-cmsd /usr/bin/cmsd
> 
> dpm-xrootd.x86_64: W: dangling-symlink /usr/bin/dpm-manager-xrootd
> /usr/bin/xrootd
> dpm-xrootd.x86_64: W: dangling-symlink /usr/bin/dpm-manager-cmsd /usr/bin/cmsd
> dpm-xrootd.x86_64: W: dangling-symlink /usr/bin/dpm-xrootd /usr/bin/xrootd
> 
> It's okay the 3 targets are provided by xrootd, ... but what is the point of
> these symbolic
> links?

I discussed this, and a solution should be possible. I added a bug here:
https://svnweb.cern.ch/trac/lcgdm/ticket/344

and it should be fixed for the next release of the component.

> dpm-xrootd-devel.x86_64: W: no-dependency-on
> dpm-xrootd/dpm-xrootd-libs/libdpm-xrootd

Added.

> Indeed dpm-xrootd-devel should contain
> Requires: dpm-xrootd%{?_isa} = %{version}-%{release}

Added.

> [+] MUST: The package must be named according to the Package Naming Guidelines.
> Based on SVN moulde name.
> [+] MUST: The spec file name must match the base package %{name}
> [+] MUST: The package must be licensed with a Fedora approved license and meet
> the Licensing Guidelines.
> ASL 2.0
> [-] MUST: The License field in the package spec file must match the actual
> license.
> The only thing anywhere to suggest a license is a GPLv3 COPYING file. The c++ 
> files and headers contain a copyright from SLAC but no hint as to what the
> license
> is. Presumably you must clarify with SLAC what the license is or was.

Not yet totally clear, but see above.

> [-] 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.
> The COPYING file is included but it's unclear if that is correct.

See above.

> [+] MUST: The spec file must be written in American English.
> [+] MUST: The spec file for the package MUST be legible.
> [-] MUST: The sources used to build the package must match the upstream source,
> as provided in the spec URL.
> I think the svn checkout resolves to
> svn export
> http://svn.cern.ch/guest/lcgdm/dpm-xrootd/glite-data-dpm-xrootd_R_2_2_2_1
> dpm-xrootd-2.2.2
> which fails.

Fixed.

> [+] MUST: The package must successfully compile and build into binary rpms on
> at least one supported architecture.
> Mock builds.
> [+] 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.
> Builds on all.
> [+] MUST: All build dependencies must be listed in BuildRequires
> [+] MUST: The spec file MUST handle locales properly. This is done by using the
> %find_lang macro.
> No locales present.
> [=] MUST: Every binary RPM package which stores shared library files (not just
> symlinks) in any of the dynamic linker's default paths, must call ldconfig in
> %post and %postun.
> 
> Your dpm-xrootd package has these but they are not needed on the devel package.
> rpmlint used to show these up.

Removed.

> [+] MUST: If the package is designed to be relocatable, the packager must state
> this fact in the request for review
> Not relocatablle
> [+] 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.
> [+] MUST: A package must not contain any duplicate files in the %files listing.
> [+] 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.
> [+] MUST: Each package must have a %clean section, which contains rm -rf
> %{buildroot} (or $RPM_BUILD_ROOT).
> [+] MUST: Each package must consistently use macros, as described in the macros
> section of Packaging Guidelines.
> [+] MUST: The package must contain code, or permissible content. This is
> described in detail in the code vs. content section of Packaging Guidelines.
> [+] MUST: Large documentation files should go in a doc subpackage.
> [+] MUST: If a package includes something as %doc, it must not affect the
> runtime of the application.
> [+] MUST: Header files must be in a -devel package.
> [+] MUST: Static libraries must be in a -static package.
> [+] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
> (for directory ownership and usability).
> [] 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.
> [=] MUST: In the vast majority of cases, devel packages must require the base
> package using a fully versioned dependency: Requires: %{name} =
> %{version}-%{release} 
> Fails as noticed by rpmlint

Fixed.

> [=] MUST: Packages must NOT contain any .la libtool archives, these should be
> removed in the spec.
> There is a .la and a .a file, drop them.

Fixed.

> [+] 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.
> [?] MUST: Packages must not own files or directories already owned by other
> packages.
> I notice this create /var/log/xroot and xrootd creates /var/log/xrootd ... Is
> that intentional?

Dropped. The directory comes with the xrootd package, and dpm-xrootd was
putting the files in a different one (that's why it was needed).

> [+] MUST: At the beginning of %install, each package MUST run rm -rf
> %{buildroot} (or $RPM_BUILD_ROOT).
> Only needed for RHEL5.
> [+] MUST: All filenames in rpm packages must be valid UTF-8.
> 
> SHOULD Items:
> [+] SHOULD: The reviewer should test that the package builds in mock.
> [+] SHOULD: The package should compile and build into binary rpms on all
> supported architectures.
> [To be done] SHOULD: The reviewer should test that the package functions as
> described.
> [=] SHOULD: If scriptlets are used, those scriptlets must be sane.
> ldconfig on devel should not be there.

Fixed.

> [=] SHOULD: Usually, subpackages other than devel should require the base
> package using a fully versioned dependency.
> See above.

Fixed.

> [] 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.
> [+] 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.
> [+] SHOULD: Packages should try to preserve timestamps of original installed
> files.
> 
> Comments beyond what is above:
> (1) CFLAGS do not look correct:
> g++ "-DPACKAGE_NAME=\"DPM xrootd\"" -DPACKAGE_TARNAME=\"dpm-xrootd\"
> -DPACKAGE_VERSION=\"0.0.0\" "-DPACKAGE_STRING=\"DPM xrootd 0.0.0\""
> -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DSTDC_HEADERS=1
> -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1
> -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1
> -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\"
> -DPACKAGE=\"dpm-xrootd\" -DVERSION=\"0.0.0\" -D_THREAD_SAFE=1 -D_REENTRANT=1
> -D_LARGEFILE_SOURCE=1 -D_FILE_OFFSET_BITS=64 -I. -I../ -I/usr/include/xrootd/
> -I /usr/include/dpm -DXRDDPM_BUILDDATE=\"121111200750\" -g -O2 -MT
> libXrdDPMXmi_la-XrdDPMXmi.lo -MD -MP -MF .deps/libXrdDPMXmi_la-XrdDPMXmi.Tpo -c
> XrdDPMXmi.cc  -fPIC -DPIC -o .libs/libXrdDPMXmi_la-XrdDPMXmi.o

Fixed (major cleanup thanks to the move to cmake).

> (2) Templates present but no configuration.
> I presume that 
> %config(noreplace) %{_sysconfdir}/sysconfig/dpm-xrd.templ
> %config(noreplace) %{_sysconfdir}/xrd.authz.cnf.templ
> %config(noreplace) %{_sysconfdir}/xrd.dpm.cf.templ
> 
> are template files and not the live files?
> Better to move or at least copy them into the used location.

Fixed as suggested.

> (3) The devel package can drop these %doc since they are in the main
>    package. On a similar node the NEWS and AUTHORS file seem
>    pointless.

Fixed as suggested.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.