[Bug 225974] Merge Review: krb5

Wed Apr 8 21:35:31 UTC 2009

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=225974

--- Comment #9 from Nalin Dahyabhai <nalin at redhat.com>  2009-04-08 17:35:29 EDT ---
(In reply to comment #2)

Okay, finally worked through most of these.  Comments in-line, apologies in
advance for the length, I'll try to cut down duplicates.

> rpmlint on SRPM:
> 
> krb5.src:109: E: prereq-use grep, info, sh-utils, /sbin/install-info
> The use of PreReq is deprecated. In the majority of cases, a plain Requires is
> enough and the right thing to do. Sometimes Requires(pre), Requires(post),
> Requires(preun) and/or Requires(postun) can also be used instead of PreReq.

Cleaned these up to Requires(post)/Requires(preun)/Requires(postun) for the
subpackages which include scriptlets.

> krb5.src:110: E: buildprereq-use autoconf, bison, e2fsprogs-devel >= 1.35,
> flex, gawk
> The use of BuildPreReq is deprecated, build dependencies are always required
> before a package can be built.  Use plain BuildRequires instead.

Done.

> krb5.src:145: W: unversioned-explicit-obsoletes krb5-configs
> The specfile contains an unversioned Obsoletes: token, which will match all
> older, equal and newer versions of the obsoleted thing.  This may cause update
> problems, restrict future package/provides naming, and may match something it
> was originally not inteded to match -- make the Obsoletes versioned if
> possible.

Removed the obsoletes, as the package being obsoleted last existed ca. RHL 6.2,
which was long enough ago that it's not worth keeping in there.

> krb5.src:1076: W: macro-in-%changelog _var
> Macros are expanded in %changelog too, which can in unfortunate cases lead to
> the package not building at all, or other subtle unexpected conditions that
> affect the build.  Even when that doesn't happen, the expansion results in
> possibly "rewriting history" on subsequent package revisions and generally odd
> entries eg. in source rpms, which is rarely wanted.  Avoid use of macros in
> %changelog altogether, or use two '%'s to escape them, like '%%foo'.

Cleaned up all instances of unescaped macros in the changelog.

> krb5.src:1399: E: use-of-RPM_SOURCE_DIR
> You use $RPM_SOURCE_DIR or %{_sourcedir} in your spec file. If you have to use
> a directory for building, use $RPM_BUILD_ROOT instead.

The source package includes scripts.  Calling them by name is more readable for
me than calling them using macros such as %{sourceNN}, which is the only other
alternative I know of.

> krb5.src:1490: W: make-check-outside-check-section : make check
> TMPDIR=%{_tmppath}
> Make check or other automated regression test should be run in %check, as they
> can be disabled with a rpm macro for short circuiting purposes.

Last check, the test suite didn't run properly in buildroots because there's no
controlling terminal.  It's effectively commented out, though.

> krb5.src: W: mixed-use-of-spaces-and-tabs (spaces: line 325, tab: line 1400)
> The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
> annoyance.  Use either spaces or tabs for indentation, not both.

This feels needlessly pedantic to me.  The scriptlets consistently use tabs for
indentation, and the changelog consistently uses spaces.

> krb5.src: W: patch-not-applied Patch26: krb5-1.3.2-efence.patch
> A patch is included in your package but was not applied. Refer to the patches
> documentation to see what's wrong.

Patch includes comment at its top which notes that it's only useful in
debugging setups.

> krb5.src: W: patch-not-applied Patch55: krb5-1.6.1-empty.patch
> A patch is included in your package but was not applied. Refer to the patches
> documentation to see what's wrong.

Patch includes comment at its top which notes that it's a work-in-progress.

> krb5.src: W: patch-not-applied Patch57: krb5-1.6.2-login_chdir.patch
> A patch is included in your package but was not applied. Refer to the patches
> documentation to see what's wrong.

Patch includes comment at its top which notes that it's a work-in-progress.

> krb5.src: W: patch-not-applied Patch64: krb5-ok-as-delegate.patch
> A patch is included in your package but was not applied. Refer to the patches
> documentation to see what's wrong.

Patch includes comment at its top noting that it's an ABI change and waiting on
feedback/approval/motion upstream.

> krb5.src: W: patch-not-applied Patch70: krb5-trunk-kpasswd_tcp2.patch
> A patch is included in your package but was not applied. Refer to the patches
> documentation to see what's wrong.

This is an alternate version of patch59, waiting on feedback/approval/motion
upstream.
> krb5.src: W: summary-ended-with-dot The Kerberos network authentication system.
> Summary ends with a dot.

Fixed.

> krb5.src: W: strange-permission krb5kdc.init 0755
> A file that you listed to include in your package has strange permissions.
> Usually, a file should have 0644 permissions.

I'm gonna need help from CVS admins for this one -- the permissions are set at
checkout-time.

> krb5.src: W: strange-permission krb5-tex-pdf.sh 0755
> A file that you listed to include in your package has strange permissions.
> Usually, a file should have 0644 permissions.

We run this script during the build.  We could take the execute bit off and run
a shell with the script as its argument, I guess.

> krb5-devel.i386: W: spurious-executable-perm
> /usr/share/doc/krb5-devel-1.6.3/krb5-protocol/draft-jaganathan-rc4-hmac-03.txt
> The file is installed with executable permissions, but was identified as one
> that probably should not be executable.  Verify if the executable bits are
> desired, and remove if not.

Fixed.

> krb5-libs.i386: W: hidden-file-or-dir /usr/kerberos/man/man5/.k5login.5.gz
> The file or directory is hidden. You should see if this is normal, and delete
> it from the package if not.
> 
> ???????????

This is the man page for the use of ~/.k5login.  Not really sure what to do
with it.

> krb5-pkinit-openssl.i386: W: no-documentation
> The package contains no documentation (README, doc, etc). You have to include
> documentation files.
> 
> Fix if possible.

It doesn't look like there are any docs specific to this module.

> krb5-server.i386: E: executable-marked-as-config-file /etc/rc.d/init.d/krb524
> Executables must not be marked as config files because that may prevent
> upgrades from working correctly. If you need to be able to customize an
> executable, make it for example read a config file in /etc/sysconfig.

File dropped (krb4-specific).

> krb5-server.i386: E: executable-marked-as-config-file /etc/rc.d/init.d/krb5kdc
> Executables must not be marked as config files because that may prevent
> upgrades from working correctly. If you need to be able to customize an
> executable, make it for example read a config file in /etc/sysconfig.
> 
> krb5-server.i386: E: executable-marked-as-config-file /etc/rc.d/init.d/kadmin
> Executables must not be marked as config files because that may prevent
> upgrades from working correctly. If you need to be able to customize an
> executable, make it for example read a config file in /etc/sysconfig.
> 
> krb5-server.i386: E: executable-marked-as-config-file /etc/rc.d/init.d/kprop
> Executables must not be marked as config files because that may prevent
> upgrades from working correctly. If you need to be able to customize an
> executable, make it for example read a config file in /etc/sysconfig.

Fixed.

> krb5-server.i386: W: conffile-without-noreplace-flag /etc/rc.d/init.d/kadmin
> A configuration file is stored in your package without the noreplace flag. A
> way to resolve this is to put the following in your SPEC file:
> %config(noreplace) /etc/your_config_file_here
> 
> krb5-server.i386: W: conffile-without-noreplace-flag /etc/rc.d/init.d/kprop
> A configuration file is stored in your package without the noreplace flag. A
> way to resolve this is to put the following in your SPEC file:
> %config(noreplace) /etc/your_config_file_here
> 
> krb5-server.i386: W: conffile-without-noreplace-flag /etc/rc.d/init.d/krb5kdc
> A configuration file is stored in your package without the noreplace flag. A
> way to resolve this is to put the following in your SPEC file:
> %config(noreplace) /etc/your_config_file_here

Fixed by not marking them as configuration files.

> krb5-server.i386: W: no-reload-entry /etc/rc.d/init.d/kprop
> In your init script (/etc/rc.d/init.d/your_file), you don't have a 'reload'
> entry, which is necessary for good functionality.
> 
> Fix.  If the software doesn't support it, just mirror the restart entry.

Done.

> krb5-server-ldap.i386: E: library-without-ldconfig-postin
> /usr/lib/libkdb_ldap.so.1.0
> This package contains a library and provides no %post scriptlet containing a
> call to ldconfig.
> 
> krb5-server-ldap.i386: E: library-without-ldconfig-postun
> /usr/lib/libkdb_ldap.so.1.0
> This package contains a library and provides no %postun scriptlet containing a
> call to ldconfig.
> 
> Probably needs fixing.

Fixed.

> krb5-workstation.i386: E: setuid-binary /usr/kerberos/bin/ksu root 04755
> The file is setuid, this may be dangerous, especially if this  file is setuid
> root.
> 
> Necessary, I suspect.

Yup.  For a while we took the setuid bit off, but it's actually useless without
it, and the bug reports were rarely friendly.

> krb5-workstation.i386: W: unstripped-binary-or-object /usr/kerberos/bin/ksu
> 
> Fix if possible.

The buildroot strip script misses setuid applications.  I'm actually not sure
if that's intentional or not.

> krb5-workstation-clients.i386: E: info-files-without-install-info-postin
> /usr/share/info/krb5-user.info.gz
> This package contains info files and provides no %post scriptlet containing a
> call to install-info.
> 
> krb5-workstation-clients.i386: E: info-files-without-install-info-postun
> /usr/share/info/krb5-user.info.gz
> This package contains info files and provides no %postun scriptlet containing
> a call to install-info.
> 
> Fix.

Done.

> krb5-workstation-clients.i386: W: unstripped-binary-or-object
> /usr/kerberos/bin/v4rcp
> 
> Fix if possible.

krb4-specific, so it's gone.

> krb5-workstation-servers.i386: E: postin-without-chkconfig
> /etc/rc.d/init.d/krb524
> The package contains an init script but doesn't call chkconfig in its %post.
> 
> krb5-workstation-servers.i386: E: init-script-without-chkconfig-preun
> /etc/rc.d/init.d/krb524
> The package contains an init script but doesn't contain a %preun with a call
> to chkconfig.
> 
> Probably needs fixing.

The logic for figuring out which subpackage should chkconfig --del the symlinks
would have been a nightmare.  But it's krb4-specific, so triggering on removal
of the older version of either subpackage isn't that bad.  (Except for the part
where it's a trigger, and triggers are best avoided if possible.)

...

Okay, I think that's most of it.  Can I trouble you to poke at it again and see
what I've missed that still needs to be addressed?

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