Bug 991314

Hi,

I think you can add your name in Bugzilla ;)

This is an informal review:

0. I think you can improve the summary of this library, not just "scrypt() library".

1. Version field has a "a", is it an Alpha release? If so, see:

http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Version_Tag

As rpmlint said:

libscrypt.i686: W: incoherent-version-in-changelog 1.1 ['1.1a-1.fc20', '1.1a-1']

2. Remove rm -rf $RPM_BUILD_ROOT in %install section.

3. Change

PREFIX=/usr

to

PREFIX=%{_prefix}

4. We don't recommend shipping static libraries, see:


So please change

find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';'

to 

find $RPM_BUILD_ROOT -name '*.*a' -exec rm -f {} ';'

Oh, forgot a link for static library:

http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

Hi Christopher,

Many thanks for this review. Going through your points:

Name: Done.

0: I had a paragraph here, and rpmlint told me to write something smaller :p I think I've come up with a happy medium now.

1: It's definitely not an alpha release. I made a one line Makefile change from v1.1 to help this packaging process and called it 1.1a - apparently a bad move. I've just tagged a newer version as 1.12 which should resolve this issue.

2. Done. This was put here by the wizard.

3. Done. Definitely an improvement.

4. Done. However, I don't think it should have been there in the first place, so the .a file is no longer installed by "make install". This has been documented on my github page in the changenotes. Ironically, this is a revert to an earlier configuration, however, a packager for another distribution originally requested this.

Hopefully I've addressed everything in one go. rpmlint only warns about spelling:

[fedora@ip-172-31-20-108 rpmbuild]$ rpmlint ./SPECS/libscrypt.spec ./SRPMS/libscrypt-1.12-1.fc19.src.rpm
libscrypt.src: W: spelling-error Summary(en_US) scrypt -> crypt, crypts, script
libscrypt.src: W: spelling-error %description -l en_US scrypt -> crypt, crypts, script
1 packages and 1 specfiles checked; 0 errors, 2 warnings.

Koji appeared to run well:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5691246
5691246 build (f19, libscrypt-1.12-1.fc19.src.rpm) completed successfully

I've updated my testing reference to demonstrate Fedora as a confirmed installation:
http://www.lolware.net/libscrypttesting.txt

The spec file has been updated at the original URL: http://www.lolware.net/libscrypt.spec

And a new version appropriate SRPM is here:
http://www.lolware.net/libscrypt-1.12-1.fc19.src.rpm


Many thanks for the review, hopefully I've addressed any concerns.

* Run rpmlint (or rpmlint -I for more helpful output) on the src.rpm and all
built rpms. Feel free to ignore obvious false positives in the report, but fix
anything else. Preferably add a comment here about whether/when you think what
rpmlint reports is correct or incorrect.


> Summary:        Linux scrypt shared library

Imagine all packages added "Linux" at the beginning of the summary. That's really superfluous. Mentioning that it's a "shared" lib currently is also uninteresting, since Fedora focuses on shared libs.

However, you could spend a few more words on summing up what's included in the package. You don't need to restrict yourself to just 2-3 words, e.g. the first sentence from the %description sounds good:

  Summary: Library that implements the secure password hashing function "scrypt"

Or more generic:

  Summary: Password hash crypto library


> %description
> A shared library that implements the secure password hashing
> function "scrypt". 

Not really a full sentence. Better:

  %description
  This is a library that implements the secure password hashing function "scrypt". 


* In the build.log:

> + make -j5
> gcc -O2 -Wall -g -D_FORTIFY_SOURCE=2 -fstack-protector -fPIC   -c -o 
> crypto_scrypt-nosse.o crypto_scrypt-nosse.c

It doesn't use Fedora's compiler %{optflags} yet:
https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags


> -Wl,-soname,libscrypt.so.0 

Something's wrong there. The package (your koji build) doesn't provide the SONAME yet:

$ rpm -qp --provides  libscrypt-1.12-1.fc19.x86_64.rpm 
libscrypt = 1.12-1.fc19
libscrypt(x86-64) = 1.12-1.fc19

That's a major blocker.

The -devel package depends on it, but it's not provided. The -devel package would not even install.

> $ rpmls -p libscrypt-debuginfo-1.12-1.fc19.x86_64.rpm 
> $

The -debuginfo package is empty and useless.

> $ rpmls -p libscrypt-1.12-1.fc19.x86_64.rpm 
> -rw-r--r--  /usr/lib64/libscrypt.so.0

chmod 0755

Again, thank you for the review.

I've used the summary and description you've suggested in my updated spec file with this.

I've also configured it to use the %{optflag} macro, which I've brought back to what I was trying to achieve with the original CLFAGS by adding %global _hardened_build 1. Based on the build output it appears to be using it.

Regarding the permissions/chmod issue, I saw this more as an upstream problem  (make install should have managed this) and fixed it there in v1.13. Note, I haven't actually tagged v1.13 at the official distribution yet. I really don't want to end up with 49 "stable releases" due to troubleshooting on the packaging. The commit that the SRPM was created from will get this tag the moment I hear we look ready to go.

I'm not sure what to make of the SONAME issue. I fixed the issues above, then ran your commands and the soname seems to be there:
[fedora@ip-172-31-20-108 SPECS]$ rpm -qp --provides /home/fedora/rpmbuild/RPMS/x86_64/libscrypt-1.13-1.fc19.x86_64.rpm
libscrypt = 1.13-1.fc19
libscrypt(x86-64) = 1.13-1.fc19
libscrypt.so.0()(64bit)
libscrypt.so.0(libscrypt)(64bit)

I must admit I don't fully understand this issue so any advice here would be appreciated.

Likewise, the debuginfo seems full of files now:
[fedora@ip-172-31-20-108 SPECS]$ rpmls -p /home/fedora/rpmbuild/RPMS/x86_64/libscrypt-debuginfo-1.13-1.fc19.x86_64.rpm
rpm: no arguments given for query
drwxr-xr-x  /usr/lib/debug
drwxr-xr-x  /usr/lib/debug/.build-id
drwxr-xr-x  /usr/lib/debug/.build-id/97
...


In evaluating rpmlint:
[fedora@ip-172-31-20-108 rpmbuild]$ rpmlint --verbose ./SPECS/libscrypt.spec ./SRPMS/libscrypt-1.13-1.fc19.src.rpm ./RPMS/x86_64/*

Numerous spelling warnings: Spelling is correct, scrypt just happens to look like "script" or "crypt"

libscrypt.src: W: invalid-url Source0: https://github.com/technion/libscrypt/archive/v1.13.tar.gz HTTP Error 404: Not Found

As above, the second it looks like we've resolved everything else I'll tag 1.13 and this error will go away.

W: no-documentation
I've made some changed to the spec file and believe this issue is resolved for the main rpm. -devel also claims to have "no documentation", but the README.md covers both elements of the RPM and such I don't consider this an issue.

Yet another SRPM update:
http://www.lolware.net/libscrypt-1.13-1.fc19.src.rpm
Updated spec file is in original location.

New koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5699750

Additional information regarding testing of the current RPMs:

[fedora@ip-172-31-20-108 x86_64]$ sudo rpm -iv libscrypt-1.13-1.fc19.x86_64.rpm Preparing packages...
libscrypt-1.13-1.fc19.x86_64
[fedora@ip-172-31-20-108 x86_64]$ sudo rpm -iv libscrypt-devel-1.13-1.fc19.x86_64.rpm
Preparing packages...
libscrypt-devel-1.13-1.fc19.x86_64
[fedora@ip-172-31-20-108 x86_64]$ ls -l /usr/lib64/libscry*
lrwxrwxrwx. 1 root root    14 Aug  3 05:24 /usr/lib64/libscrypt.so -> libscrypt.so.0
-rwxr-xr-x. 1 root root 27688 Aug  3 03:54 /usr/lib64/libscrypt.so.0
[fedora@ip-172-31-20-108 x86_64]$ ls -l /usr/include/libscrypt.h
-rw-r--r--. 1 root root 2204 Aug  3 03:54 /usr/include/libscrypt.h

main.c is the test hardness distributed with the source.
[fedora@ip-172-31-20-108 ~]$ gcc -Wall main.c -lscrypt -o reference
[fedora@ip-172-31-20-108 ~]$ ./reference
...
TEST TWELVE: SUCCESSSFUL. Received the following from simple hash:
$s1$0e0810$tiBOLHS6jXyAJFaFmp+rFQ==$WxXBZhDqk0BtrcEDrparUjESet5sw2VpSsENNyE1aQu6C2+Xnsljvu823PjcJZAZa3284UlMreC7BUTah1v1Zw=

I think no big problems here right now.

But you still need a sponsor ;)

http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

I can't help ;)

Yes, fixing the library file permissions has also fixed the SONAME Provides.


> main.c is the test hardness distributed with the source.
> [fedora@ip-172-31-20-108 ~]$ gcc -Wall main.c -lscrypt -o reference
> [fedora@ip-172-31-20-108 ~]$ ./reference

Great! You've just found something to run in the spec file's %check section. For example:

  %check
  gcc -Wall main.c -L$(pwd) -lscrypt -o reference
  LD_LIBRARY_PATH=$(pwd) ./reference

I found that in Makefile there is a check section.

So as Michael has said, you should add a %check section below %install section:

%check
make check

Thanks for that confirmation Michael.

I've just updated the spec file as per Christopher's suggestion. Koji runs for a heck of a long time, because my reference test runs a very high complexity count hash. But it does work and the build log appears to reflect the test harness.

http://koji.fedoraproject.org/koji/taskinfo?taskID=5707849

http://kojipkgs.fedoraproject.org//work/tasks/7855/5707855/build.log

I've pushed v1.13 tag upstream so we can remove those warnings. Here's a fresh rpmlint to make sure I didn't introduce any new issues:

[fedora@ip-172-31-20-108 rpmbuild]$ rpmlint --verbose ./SPECS/libscrypt.spec ./SRPMS/libscrypt-1.13-1.fc19.src.rpm ./RPMS/x86_64/*

Again, ignoring the spelling false-positives, the only warning is "W: no-documentation" on the devel package only.

Spec file updated:
http://www.lolware.net/libscrypt.spec
SRPM:
http://www.lolware.net/libscrypt-1.13-1.fc19.src.rpm

Verification for current versions:
# sha1sum libscrypt.spec
9af9dc3964df05428e9989294cc2fdf5f885024a  libscrypt.spec
# sha1sum libscrypt-1.13-1.fc19.src.rpm
ee192d04adc83a7296a6c74a842ebdc197f64f8c  libscrypt-1.13-1.fc19.src.rpm

To assist with sponsoring, I've done two informal reviews:
https://bugzilla.redhat.com/show_bug.cgi?id=910340
https://bugzilla.redhat.com/show_bug.cgi?id=991531

I'm be looking for more on an ongoing basis.

Hi,

I have performed another review:

https://bugzilla.redhat.com/show_bug.cgi?id=993636

I think he is good enough now.

What about you?

Also: https://bugzilla.redhat.com/show_bug.cgi?id=993456

No blocker left here. The update from comment 11 fixes the packaging issues.

APPROVED

New Package SCM Request
=======================
Package Name: libscrypt
Short Description: Library that implements the secure password hashing function "scrypt"
Owners: technion
Branches: f18 f19 f20
InitialCC:

Hey....You mustn't change the fedora-review flag to + by yourself...

Please let Michael do this.

And no f20 branch available now.

Michael already did set the fedora-review flag to + in comment 16.

I accidentally removed it when I was trying to set fedora-cvs.. then immediately set it back.

f20 lists as a koji build and runs:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5792320

If I'm supposed to remove it from here, should it be a new SCM request?

fedora-review flag only can be set by others but not "you".

So you should wait for Michael to change it.

Don't worry, you have 12 hours until today's window of SCM process.

New Package SCM Request
=======================
Package Name: libscrypt
Short Description: Library that implements the secure password hashing function "scrypt"
Owners: technion
Branches: f18 f19
InitialCC:

Git done (by process-git-requests).

libscrypt-1.13-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/libscrypt-1.13-1.fc18

libscrypt-1.13-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/libscrypt-1.13-1.fc19

libscrypt-1.13-1.fc18 has been pushed to the Fedora 18 testing repository.

libscrypt-1.13-1.fc18 has been pushed to the Fedora 18 stable repository.

libscrypt-1.13-1.fc19 has been pushed to the Fedora 19 stable repository.