Bug 832504

I'll look at reviewing this here when I can.

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

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

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 .

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.

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

New Package SCM Request
=======================
Package Name: pesign
Short Description: Signing utilities for UEFI binaries.
Owners: pjones
Branches: F18
InitialCC:

*** Bug 834000 has been marked as a duplicate of this bug. ***

Git done (by process-git-requests).

No need to request f18, devel is automatic.

Package Change Request
======================
Package Name: pesign
New Branches: el6
Owners: pjones mjg59

Git done (by process-git-requests).

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.

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

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.

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.

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

This is in and done, closing.