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@xxxxxxxxx> 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@GLIBC_2.2.5 > dpm-xrootd.x86_64: W: shared-lib-calls-exit > /usr/lib64/libXrdDPMOfsAndN2N.so.0.0.0 _exit@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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review