[Bug 691818] Review Request: openpts - TCG Platform Trust Service (PTS) for embedded devices

[bugzilla at redhat.com]

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

Avesh Agarwal <avagarwa at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?(avagarwa at redhat.c |
                   |om)                         |

--- Comment #2 from Avesh Agarwal <avagarwa at redhat.com> 2011-04-13 14:46:13 EDT ---
Thanks for your review.

(In reply to comment #1)
> blockers:
> 
> * Package doesn't build in mock/koji:
> > + autoreconf -fv --install
> > autoreconf: Entering directory `.'
> > autoreconf: running: autopoint --force
> > Can't exec "autopoint": Permission denied at /usr/share/autoconf/Autom4te/FileUtils.pm line 345.
> > autoreconf: failed to run autopoint: Permission denied
> > autoreconf: autopoint is needed because this package uses Gettext
>   This might only be a missing BuildRequires on gettext*, see below...
> 

Fixed.

> * > openpts.x86_64: W: file-not-in-%lang
> /usr/share/locale/ja/LC_MESSAGES/openpts.mo
>   Use the %find_lang macro for translations, see
> https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files
>

Fixed.

> * Source0: points to a HTML page.
>   If it is possible to use something similar to
> https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net to get a
> direct URL to the tarball, this should be done.  This problem is not a blocker
> if no such option exists, obviously.
> 

Can you please check again, because for me, it points to the file and lets me
download the package?

> * Is there a reason for the explicit "Requires: trousers openssl"?
>   rpm seems to be able to correctly add automatic dependencies.  If these
>   requirements are necessary, please add a comment (per
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires )
> 

Fixed.

> * Requires(preun, post, postun) for scriptlets are missing, see
>  
> https://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_spec_file_scriptlets
> 

Fixed.

> * It seems the default configuration uses /var/lib/openpts.  If so, shouldn't
>   the directory be owned by the package?
> 

Fixed.

> * /usr/share/openpts and /usr/share/openpts/models should be owned
>   by the package.
> 

Fixed.

> non-blockers:
> > openpts.x86_64: W: no-manual-page-for-binary openpts
> > openpts.x86_64: W: no-manual-page-for-binary tpm_createkey
> > openpts.x86_64: W: no-manual-page-for-binary ptscd
> > openpts.x86_64: W: no-manual-page-for-binary rm2dot
> > openpts.x86_64: W: no-manual-page-for-binary uml2dot
> > openpts.x86_64: W: no-manual-page-for-binary ir2text
> > openpts.x86_64: W: no-manual-page-for-binary iml2aide
> > openpts.x86_64: W: no-manual-page-for-binary iml2text
> It would be nice to have man pages, but writing them is primarily upstream's
> responsibility, having man pages is not a requirement.
> 

Not fixed for the obvious reason what you stated above.

> * Including the documenation from doc/ would probably be useful to users.
>   Please also consider including ChangeLog in %doc.
> 

Fixed.

> > openpts.x86_64: W: incoherent-init-script-name ptscd ('openpts', 'openptsd')
> Something to consider... but not a hard requirement IMHO.
> 

Not fixed. They have this perhaps because there is already a binary named
openpts in the package, so to avoid confusion another name for init script.
That said, surely can talk with upstream about it.  

> * The parenthesized abbreviations in %description look a little strange to me:
>   They are not used anywhere else, so they are rather superfluous - especially
>   the PoC abbreviation.  This purely a matter of taste, of course.
> 
Partially fixed. PTS and TCG are standard abbreviations used by TCG. Removed
PoC though.

> * The correct macro for /etc/init.d is _initddir, not _initrddir.
> 
Fixed.

> * For consistency, consider using _/sbin/_chkconfig in %post
> 

Fixed.

> * The initscript should probably exit with 2, not 3, on invalid command name
>   (per the example in https://fedoraproject.org/wiki/Packaging:SysVInitScript )
>

Fixed.

> * Consider using (cp -p) and (make install DESTDIR=... INSTALL='install -p')
>   in %install to preserve timestamps
> 

Fixed.

> *
> https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

As such there is no formal bug reporting system with upstream. I will send
patches soon to upstream though.


The updated spec/srpm is as at 
http://people.redhat.com/avagarwa/files/openpts/openpts.spec
http://people.redhat.com/avagarwa/files/openpts/openpts-0.2.3-2.fc16.src.rpm

-- 
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.