Bug 832504 - Review Request: pesign - Utility for signing UEFI applications
Summary: Review Request: pesign - Utility for signing UEFI applications
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 834000 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-06-15 14:55 UTC by Peter Jones
Modified: 2013-02-03 03:00 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-02-03 03:00:39 UTC
kevin: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Peter Jones 2012-06-15 14:55:27 UTC
Spec URL: http://pjones.fedorapeople.org/pesign/pesign.spec
SRPM URL: http://pjones.fedorapeople.org/pesign/pesign-0.1-1.fc17.src.rpm
Description: Utility for signing UEFI applications
Fedora Account System Username: pjones

Comment 1 Kevin Fenzi 2012-06-15 14:56:25 UTC
I'll look at reviewing this here when I can.

Comment 2 Kevin Fenzi 2012-06-16 15:35:51 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
OK - License (GPLv2)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:

OK - BuildRequires correct
See below- Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

See below - Package compiles and builds on at least one arch. 
See below- Package has no duplicate files in %files. 
See below- Package doesn't own any directories other packages own. 
See below- Package owns all the directories it creates. 
See below- Package obey's FHS standard (except for 2 exceptions)
See below- No rpmlint output. 
See below- final provides and requires are sane.

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
OK - Should have sane scriptlets. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin

Issues: 

1. github seems to not be very sourceurl friendly. The current one is giving a 404. 
Perhaps just do a git checkout (and include comment on how to get that checkout?)

2. Does not seem to build in mock:

pe_update.c:79:1: error: conflicting types for 'pe_update'
In file included from libdpe.h:22:0,
                 from pe_update.c:20:
/builddir/build/BUILD/pesign-0.1/include/libdpe/libdpe.h:81:15: note: previous declaration of 'pe_update' was here
make[1]: *** [pe_update.o] Error 1

Fix the build and I can finish this review up. ;)

Comment 3 Peter Jones 2012-06-19 13:49:57 UTC
<i>1. github seems to not be very sourceurl friendly. The current one is giving a 404. 
Perhaps just do a git checkout (and include comment on how to get that checkout?)</i>

Yeah, it's really just a dummy url.  I'll look into why it's failing in mock.

Comment 4 Peter Jones 2012-06-19 14:25:13 UTC
I can't seem to reproduce your mock failure.  I'm using:

mock -r fedora-17-x86_64 --rebuild pesign-0.2-1.fc17.src.rpm 

Results and logs are in the same directory as the .spec and src.rpm .

Comment 5 Kevin Fenzi 2012-06-19 15:49:59 UTC
Everything builds ok on f17 in mock now. Not sure what was up. rawhide still doesn't build, but that looks like a rawhide breakage unrelated to this package.

OK - Sources match upstream md5sum:
(no md5sum, but diff -Nur shows no non timestamp differences in checkout). 

OK - Package has %defattr and permissions on files is good. 
OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
OK - final provides and requires are sane.


rpmlint says: 

pesign.src:10: W: macro-in-comment %{version}

Use %% ?

pesign.src: W: invalid-url Source0: https://github.com/vathpela/pesign/pesign-0.2.tar.bz2 HTTP Error 404: Not Found

Drop the url entirely since this is just a checkout? 
Source0: pesign-0.2.tar.bz2

pesign.x86_64: W: incoherent-version-in-changelog 0.1-1 ['0.2-1.fc17', '0.2-1']

Add changelog entry for 0.2. ;) 

pesign.x86_64: E: non-standard-dir-perm /etc/pki/pesign 0700L
pesign.x86_64: W: non-conffile-in-etc /etc/popt.d/pesign.popt
pesign.x86_64: W: no-manual-page-for-binary pesign

All can be ignored. 
Man page would be nice, but not a blocker. 

Fix up the rpmlint stuff before checking in and this package is APPROVED.

Comment 6 Peter Jones 2012-06-19 19:55:01 UTC
(In reply to comment #5)
> Everything builds ok on f17 in mock now. Not sure what was up. rawhide still
> doesn't build, but that looks like a rawhide breakage unrelated to this
> package.
> 
> OK - Sources match upstream md5sum:
> (no md5sum, but diff -Nur shows no non timestamp differences in checkout). 
> 
> OK - Package has %defattr and permissions on files is good. 
> OK - Package compiles and builds on at least one arch. 
> OK - Package has no duplicate files in %files. 
> OK - Package doesn't own any directories other packages own. 
> OK - Package owns all the directories it creates. 
> OK - Package obey's FHS standard (except for 2 exceptions)
> See below - No rpmlint output. 
> OK - final provides and requires are sane.
> 
> 
> rpmlint says: 
> 
> pesign.src:10: W: macro-in-comment %{version}
> 
> Use %% ?
> 
> pesign.src: W: invalid-url Source0:
> https://github.com/vathpela/pesign/pesign-0.2.tar.bz2 HTTP Error 404: Not
> Found
> 
> Drop the url entirely since this is just a checkout? 
> Source0: pesign-0.2.tar.bz2

Fair enough.

> pesign.x86_64: W: incoherent-version-in-changelog 0.1-1 ['0.2-1.fc17',
> '0.2-1']
> 
> Add changelog entry for 0.2. ;) 

And 0.2-3 of course :)

> pesign.x86_64: E: non-standard-dir-perm /etc/pki/pesign 0700L
> pesign.x86_64: W: non-conffile-in-etc /etc/popt.d/pesign.popt
> pesign.x86_64: W: no-manual-page-for-binary pesign
> 
> All can be ignored. 
> Man page would be nice, but not a blocker. 

Waiting on upstream for that.  Hopefully week after next :)

> Fix up the rpmlint stuff before checking in and this package is APPROVED.

Sweet, thanks.

Comment 7 Peter Jones 2012-06-21 02:10:29 UTC
New Package SCM Request
=======================
Package Name: pesign
Short Description: Signing utilities for UEFI binaries.
Owners: pjones
Branches: F18
InitialCC:

Comment 8 Peter Jones 2012-06-21 02:11:51 UTC
*** Bug 834000 has been marked as a duplicate of this bug. ***

Comment 9 Gwyn Ciesla 2012-06-21 12:51:22 UTC
Git done (by process-git-requests).

No need to request f18, devel is automatic.

Comment 10 Peter Jones 2012-10-17 15:51:54 UTC
Package Change Request
======================
Package Name: pesign
New Branches: el6
Owners: pjones mjg59

Comment 11 Kevin Fenzi 2012-10-17 15:54:11 UTC
Git done (by process-git-requests).

Comment 12 Mr-4 2012-10-21 23:20:00 UTC
I don't know whether it is just me, my system or something else, but I am experiencing quite a few problems with this package: Please refer to Bug 868581, Bug 868590, Bug 868591 and Bug 868720. 

I've got around the first 3 bugs, but the last one is a deal-breaker for me - I can't sign anything! 

The version I originally used was 0.99 (sourced from the srpm provided in Rawhide), but since it cannot compile at all, I fetched the latest source from git as of yesterday 20 Oct 2012, archived it, and then dropped it in my SOURCE directory for rpmbuild to pick up.

Comment 13 Peter Jones 2012-10-22 14:52:20 UTC
(In reply to comment #12)
> I don't know whether it is just me, my system or something else, but I am
> experiencing quite a few problems with this package: Please refer to Bug
> 868581, Bug 868590, Bug 868591 and Bug 868720. 
> 
> I've got around the first 3 bugs, but the last one is a deal-breaker for me
> - I can't sign anything! 
> 
> The version I originally used was 0.99 (sourced from the srpm provided in
> Rawhide), but since it cannot compile at all, I fetched the latest source
> from git as of yesterday 20 Oct 2012, archived it, and then dropped it in my
> SOURCE directory for rpmbuild to pick up.

It appears you're not having *any* problems with the package, but instead having problems building from git on some older (or other) distribution that this package isn't included in.

Comment 14 Mr-4 2012-10-22 16:13:12 UTC
Please refer to the bugs I indicated above, particularly Bug 868720. As I already pointed out in Comment 12 above, although I could get around the first 3 bugs, the last one (Bug 868720) is a show-stopper for me.

As for your last sentence in Comment 13, as far as I am aware pesign is still in rawhide and it is not provided as part of any distros (not yet, anyway), nor does it specify anywhere - either in the README, TODO, its .spec file or anywhere else - that it needs to be build for, or using specific, distribution.

Finally, as far as I can read in the original submission, this is a review request, which is precisely what I have done with my Comment 12 above.

Comment 15 Kevin Fenzi 2012-10-22 16:27:13 UTC
This bug is for the packaging of the application to make sure it meets Fedora's standards in packaging. 

It does, it's been approved. 

Bugs in the package should now be filed seperately (as you have done) and worked there.

Comment 16 Mr-4 2012-10-23 23:43:07 UTC
(In reply to comment #15)
> Bugs in the package should now be filed seperately (as you have done) and
> worked there.
See Bug 869447 and Bug 869450 I just submitted - that's in addition to what I have already posted.

Comment 17 Kevin Fenzi 2013-02-03 03:00:39 UTC
This is in and done, closing.


Note You need to log in before you can comment on or make changes to this bug.