Fedora Merge Review: gettext http://cvs.fedora.redhat.com/viewcvs/devel/gettext/ Initial Owner: petersen
Assigning to me.
Created attachment 149807 [details] rpmlint log for gettext 0.16.1-5 on FC-devel i386 Well, for 0.16.1-5: Some cleanup seems to be needed. A spec file * Summary: - Please don't end with dots. * Prereq - Deprecated. Use Requires(post) and Requires(preun) * Source2 - SOURCE files should not have executable permission. ? BuildRoot tag - This BuildRoot does not meet current Fedora policy. * Parallel make - Please check if parallel make is possible. * %{makeinstall} macro - Avoid this if possible. * Timestamps --------------------------------------------- install -m 755 %SOURCE2 ${RPM_BUILD_ROOT}/%{_bindir}/msghack --------------------------------------------- - Change to "install -p -m 755'. This is a script is keeping timestamp is recommended. - Well, perhaps also for this package --------------------------------------------- make install INSTALL="%{__install} -p" ..... --------------------------------------------- works well to keep timestamps on some files. * gettext mo files --------------------------------------------- rm -f $RPM_BUILD_DIR/%{name}-%{version}/trans.list pushd %{buildroot}/%{_datadir}/locale for foo in `find . -maxdepth 1 -mindepth 1 -type d` ; do lang=`echo $foo | cut -c 3-` echo "%lang($lang) %{_datadir}/locale/$foo/*/*" >> \ $RPM_BUILD_DIR/%{name}-%{version}/trans.list done popd --------------------------------------------- Cannot this be treated by %find_lang macro? * One line command in %postun etc... - Please use "-p" option. B. File check - Directory ownership issue ---------------------------------------------- [tasaka1@localhost gettext]$ ( for f in `rpm -ql gettext gettext-devel | sort` ; do if [ -d $f ] ; then for g in `rpm -qf $f` ; do echo -e "$f\t$g" ; done ; fi ; done ) | sed -e '/^[^ \t][^ \t]*\tgettext.*/d' /usr/share/locale/en@boldquot filesystem-2.4.2-1.fc7 /usr/share/locale/en@boldquot/LC_MESSAGES filesystem-2.4.2-1.fc7 /usr/share/locale/en@quot filesystem-2.4.2-1.fc7 /usr/share/locale/en@quot/LC_MESSAGES filesystem-2.4.2-1.fc7 ---------------------------------------------- - Static archive * Would you explain why static archive is needed, or just split these to -static subpackage? C. rpmlint issue ------ This is attached -------- NOTES: - Please use %% in %changelog for macros. - Would you explain if the status of --------------------------------------------- unversioned-explicit-provides devel(libintl) --------------------------------------------- is proper (and why this is required)? - Would you explain the status of /usr/lib/preloadable_libintl.so? * Currently this has 0644 permission * This is not stripped, debuginfo rpm does not contain the debug information for this file. - There are many 'spurious-executable-perm' complaint. - And somes are 'wrong-script-interpreter' - Check if some zero-length files are needed. D. NOTES: - /usr/share/aclocal is not owned by this package and -devel package has some files under this directory. Generally this status is not allowed, however, it is still under discussion about * should all packages which have some files under /usr/share/aclocal require "automake" to satisfy the directory ownership requirement? * or should all packages own this directory? * or should filesystem own this directory? So for now I leave this at it is.
> * Summary: > - Please don't end with dots. > > * Prereq > - Deprecated. Use Requires(post) and Requires(preun) fixing > * Source2 > - SOURCE files should not have executable permission. Not sure how to fix this: the python script is executable in cvs. > ? BuildRoot tag > - This BuildRoot does not meet current Fedora policy. Fixing. > * Parallel make > - Please check if parallel make is possible. I will try a build with parallel make. > * %{makeinstall} macro > - Avoid this if possible. replacing > * Timestamps > --------------------------------------------- > install -m 755 %SOURCE2 ${RPM_BUILD_ROOT}/%{_bindir}/msghack > --------------------------------------------- > - Change to "install -p -m 755'. This is a script is keeping timestamp > is recommended. > - Well, perhaps also for this package > --------------------------------------------- > make install INSTALL="%{__install} -p" ..... > --------------------------------------------- > works well to keep timestamps on some files. fixing and adding > * gettext mo files > Cannot this be treated by %find_lang macro? Will look into this. > * One line command in %postun etc... > - Please use "-p" option. fixing > B. File check > - Directory ownership issue > /usr/share/locale/en@boldquot filesystem-2.4.2-1.fc7 > /usr/share/locale/en@boldquot/LC_MESSAGES filesystem-2.4.2-1.fc7 > /usr/share/locale/en@quot filesystem-2.4.2-1.fc7 > /usr/share/locale/en@quot/LC_MESSAGES filesystem-2.4.2-1.fc7 removing dirs > - Static archive > * Would you explain why static archive is needed, or just split > these to -static subpackage? I added --disable-static for now. > C. rpmlint issue > - Please use %% in %changelog for macros. fixing > - Would you explain if the status of > unversioned-explicit-provides devel(libintl) > is proper (and why this is required)? No idea. Probably some historical artefact. Perhaps it can just be removed? > - Would you explain the status of /usr/lib/preloadable_libintl.so? > * Currently this has 0644 permission Let me look into it. > - There are many 'spurious-executable-perm' complaint. > - And somes are 'wrong-script-interpreter' > - Check if some zero-length files are needed. To be honest I am tempted just to remove all the examples completely from gettext-devel or at least move them to another subpackage. > D. NOTES: > - /usr/share/aclocal is not owned by this package and > -devel package has some files under this directory. > > Generally this status is not allowed, however, it is still under > discussion about Ok. Let me update here again when the changes are in cvs and the buildsystem.
(In reply to comment #3) > > * Source2 > > - SOURCE files should not have executable permission. > > Not sure how to fix this: the python script is executable in cvs. Well, it seems that to fix this you have to once remove this script from cvs and readd?? If so, leave this as it is for now. > > - Would you explain if the status of > > unversioned-explicit-provides devel(libintl) > > is proper (and why this is required)? > > No idea. Probably some historical artefact. Perhaps it can just be removed? Well, on my system 'devel(libintl)' is not required by any package. > > - There are many 'spurious-executable-perm' complaint. > > - And somes are 'wrong-script-interpreter' > > - Check if some zero-length files are needed. > > To be honest I am tempted just to remove all the examples completely from > gettext-devel or at least move them to another subpackage. Well, these files doesn't seem to be needed... Other points: * It may be better that "./ChangeLog" is added to %doc. Now I am checking all sources...
From looking at sources: * There are some test files under -------------------------------------- ./autoconf-lib-link/tests/ ./gettext-runtime/tests/ ./gettext-tools/tests/ -------------------------------------- Can any tests be done using the files under the directories above at %check stage?
(In reply to comment #3) > > * gettext mo files > > Cannot this be treated by %find_lang macro? > > Will look into this. Adding find_lang macros. > > - Would you explain the status of /usr/lib/preloadable_libintl.so? > > * Currently this has 0644 permission > > Let me look into it. I sent a mail upstream to ask about it. Currently I set it 755 in the spec file. I note however that LD_PRELOAD=/usr/lib/preloadable_libintl.so seems to work also when the file is 644. (In reply to comment #4) > Well, it seems that to fix this you have to once remove > this script from cvs and readd?? > If so, leave this as it is for now. Ok, thanks. > Well, on my system 'devel(libintl)' is not required by > any package. Ok, I removed it. > > > - There are many 'spurious-executable-perm' complaint. > > > - And somes are 'wrong-script-interpreter' > > > - Check if some zero-length files are needed. > > > > To be honest I am tempted just to remove all the examples completely from > > gettext-devel or at least move them to another subpackage. > Well, these files doesn't seem to be needed... I'm removing examples. > Other points: > * It may be better that "./ChangeLog" is added to %doc. > Now I am checking all sources... I added it to the -devel package for now, not sure how useful it is though. (In reply to comment #5) > * There are some test files under : > Can any tests be done using the files under the directories > above at %check stage? I am going to add "make check" for now, and a patch for one test that currently fails (reported upstream). :-/ I am not sure it can always be turned on, but it is good to have it documented in the spec file anyway. Also the checks take quite a time to run. I haven't used %check before but it does seem a little intrusive, for example "make install-short" also runs it for me. I committed the changes to cvs: they should be in gettext-0.16.1-6.fc7.
Created attachment 149911 [details] mock build log of gettext-0.16.1-6 on FC-devel i386 mockbuild of ettext-0.16.1-6 on FC-devel i386 failed on FC-devel i386. log says: ----------------------------------------------- make[3]: Entering directory `/builddir/build/BUILD/gettext-0.16.1/gettext-tools/m4' make[3]: Nothing to be done for `install-exec-am'. test -z "/var/tmp/gettext-0.16.1-6.fc7-Z20472/usr/share/aclocal" || /bin/mkdir -p "/var/tmp/gettext-0.16.1-6.fc7-Z20472/var/tmp/gettext-0.16.1-6.fc7-Z20472/usr/share/aclocal" /usr/bin/install -p -m 644 '../../autoconf-lib-link/m4/lib-ld.m4' '/var/tmp/gettext-0.16.1-6.fc7-Z20472/var/tmp/gettext-0.16.1-6.fc7-Z20472/usr/share/aclocal/lib-ld.m4' -----------------------------------------------
Oh, spec file seems updated again. I will check it later.
Created attachment 149922 [details] normal rpmbuild log of 0.16.1-6 on FC-devel i386 * About rpmbuild as non-root users - Well, as the attached log shows, currently normal rpmbuild fails when it is done by non-root users, more precisely, by users who do not have /sbin in their path. After explicitly adding /sbin to PATH, rpmbuild succeeds. Perhaps the related points are in ./gettext-runtime/libasprintf/Makefile.in (other files may be also related) ------------------------------------------------- 868 @$(POST_INSTALL) 869 @if (install-info --version && \ 870 install-info --version 2>&1 | sed 1q | grep -i -v debian) >/dev/null 2>&1; then \ 871 list='$(INFO_DEPS)'; \ 872 for file in $$list; do \ 873 relfile=`echo "$$file" | sed 's|^.*/||'`; \ 874 echo " install-info --info-dir='$(DESTDIR)$(infodir)' '$(DESTDIR)$(infodir)/$$relfile'";\ 875 install-info --info-dir="$(DESTDIR)$(infodir)" "$(DESTDIR)$(infodir)/$$relfile" || :;\ 876 done; \ 877 else : ; fi ------------------------------------------------- Well, when "install-info" is in their path, install-info is executed and %{buildroot}/usr/share/info/dir is created, then ------------------------------------------------- 87 88 rm ${RPM_BUILD_ROOT}%{_infodir}/dir 89 ------------------------------------------------- in spec file succeeds. However when install-info is not in their path, dir file is not created and this fails. NOTE: - Currently mockbuild does: -------------------------------------------------------- DEBUG: Executing /usr/sbin/mock-helper chroot /var/lib/mock/fedora-development-i386-core/root /sbin/runuser - root -c "cd /;/sbin/runuser -c 'rpmbuild --rebuild --target i386 --nodeps /builddir/build/SRPMS/gettext-0.16.1-6.fc7.src.rpm' mockbuild" -------------------------------------------------------- So when mockbuild is done, the path is set as same as which root sets. So: * The most simple way is just to "rm -f" * rpmlint - rpmlint for 0.16.1-6 is ---------------------------------------------- W: gettext unstripped-binary-or-object /usr/lib/preloadable_libintl.so E: gettext no-ldconfig-symlink /usr/lib/preloadable_libintl.so W: gettext-devel unused-direct-shlib-dependency /usr/lib/libasprintf.so.0.0.0 /lib/libm.so.6 E: gettext-devel zero-length /usr/share/doc/gettext-devel-0.16.1/javadoc2/package-list ----------------------------------------------- - For the first one (unstripped-binary-or-object), rpmbuild doesn't strip libraries when libraries does not have executable permissions when %install stage is done (adding %attr does not make sense for stripping) To strip this, the permission of preloadable_libintl.so has to be changed explicitly to 0755. - I don't know well about preloadable_libintl.so, however can the second rpmlint error (about ldconfig symlink) ignored? - What is the file "package-list"?
(In reply to comment #9) > * The most simple way is just to "rm -f" ok > W: gettext unstripped-binary-or-object /usr/lib/preloadable_libintl.so | > - For the first one (unstripped-binary-or-object), rpmbuild > doesn't strip libraries when libraries does not have > executable permissions when %install stage is done > (adding %attr does not make sense for stripping) > > To strip this, the permission of preloadable_libintl.so > has to be changed explicitly to 0755. fixing > E: gettext no-ldconfig-symlink /usr/lib/preloadable_libintl.so | > - I don't know well about preloadable_libintl.so, however > can the second rpmlint error (about ldconfig symlink) > ignored? The explanation reads: '''The package should not only include the shared library itself, but also the symbolic link which ldconfig would produce. (This is necessary, so that the link gets removed by rpm automatically when the package gets removed, even if for some reason ldconfig would not be run at package postinstall phase.)''' There is no symlink for preloadable_libintl.so since it is not versioned, so I think it can be ignored. > E: gettext-devel zero-length > /usr/share/doc/gettext-devel-0.16.1/javadoc2/package-list | > - What is the file "package-list"? I don't know - I can try to find out. > W: gettext-devel unused-direct-shlib-dependency /usr/lib/libasprintf.so.0.0.0 > /lib/libm.so.6 This seems to be due to the libtool in gettext-runtime/libasprintf/. Not sure if it is worth fixing.
(In reply to comment #10) > > W: gettext-devel unused-direct-shlib-dependency /usr/lib/libasprintf.so.0.0.0 > > /lib/libm.so.6 > > This seems to be due to the libtool in gettext-runtime/libasprintf/. > Not sure if it is worth fixing. Fix this if you want. Currently this warning is ignored. This means that linkage against libm.so.6 is not needed.
I fixed infodir/dir removal and the perms on the preload module in gettext-0.16.1-7.fc7.
Well, * please check if "/usr/share/doc/gettext-devel-0.16.1/javadoc2/package-list" is needed. Other things are okay. Closing.
Thanks for the review. (In reply to comment #13) > * please check if > "/usr/share/doc/gettext-devel-0.16.1/javadoc2/package-list" is needed. I asked some Java people about it and one possibility is that it may be a gjdoc bug. I was recommended to try java-1.5.0-gcj and sinjdoc. So I will do that.
Hrrm, actually I just noticed package-list is part of the upstream tarball, so this may be an upstream issue actually.