Bug 1012391 - Review Request: makepasswd - Generates (pseudo-)random passwords of a desired length
Review Request: makepasswd - Generates (pseudo-)random passwords of a desired...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-26 07:52 EDT by Johan Swensson
Modified: 2014-01-03 16:04 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-01-03 16:04:26 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
bugs.michael: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Johan Swensson 2013-09-26 07:52:41 EDT
Spec URL: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM URL: http://kupo.se/pub/makepasswd/makepasswd-0.5.1-1.fc18.src.rpm
Description: Makepasswd generates (pseudo-)random passwords of a desired length.
It is available under the GPL version 3.

Fedora Account System Username: opuk
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5986644

This is my first package submitted for Fedora and I'm in need of a sponsor.
Comment 1 Christopher Meng 2013-09-26 10:24:46 EDT
Informal review:

1. Source0:	http://people.defora.org/~khorben/projects/makepasswd/makepasswd-0.5.1.tar.gz

You should use:

Source0:	http://people.defora.org/~khorben/projects/makepasswd/%{name}-%{version}.tar.gz

So when you update it to newer version you won't be tired of handling URL again.

2. Use macro:

/usr/bin --> %{_bindir}

/usr/share/man/man1 --> %{_mandir}/man1

3. install -m 755 src/makepasswd %{buildroot}/usr/bin
install -m 644 doc/makepasswd.1 %{buildroot}/%{_mandir}/man1

You forgot to preserve the timestamp, guidelines tell us that it's a MUST.

4. %defattr(-, root, root -) is not needed now.

5. Don't mark manpages as %doc.
Comment 2 Johan Swensson 2013-09-26 10:47:58 EDT
Thank you Christopher. I have made changes accordingly.

SPEC: http://kupo.se/pub/makepasswd/makepasswd-2.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.1-2.fc18.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5986644
Comment 3 Terje Røsten 2013-09-26 14:58:18 EDT
> SPEC: http://kupo.se/pub/makepasswd/makepasswd-2.spec

Don't version the file name, use makepasswd.spec always.

> Source0:	http://people.defora.org/~khorben/projects/makepasswd/%{name}-%{version}.tar.gz

You can drop %{name} macro, the important thing is %{version}

> Patch0: db2man.sh.diff

Indent correctly

> BuildRequires:	docbook-style-xsl libxslt

Use only one buildreq per line.

> %description
> Makepasswd generates (pseudo-)random passwords of a desired length. 
> It is available under the GPL version 3.

License info not needed in %description

> cd doc

I prefer pushd doc

> # make install DESTDIR=$RPM_BUILD_ROOT/usr/bin

Remove.


> %{_mandir}/man1/makepasswd.1.gz

Drop .gz, just use %{_mandir}/man1/makepasswd.1* (conpression from rpmbuild might change).

Any license files in %files? Use %doc macro.

%changelog
* Mon Sep 26 2013 Johan Swensson <kupo@kupo.se> 0.5.1-2
- package fixes with input from informal review

Insert empty line here.

* Mon Sep 26 2013 Johan Swensson <kupo@kupo.se> 0.5.1-1
- initial build


> Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5986644
From build log:

cc   -W -Wall -g -O2 -pedantic -c makepasswd.c
cc   -W -Wall -g -O2 -pedantic -c md5c.c
cc -o makepasswd makepasswd.o md5c.o  -lcrypt

Correct build flags is not used, please fix.
Comment 4 Terje Røsten 2013-09-26 15:01:58 EDT
One more:

warning: bogus date in %changelog: Mon Sep 26 2013 Johan Swensson <kupo@kupo.se> 0.5.1-1

Sep 26 20113 is Thu, not Mon.
Comment 5 Johan Swensson 2013-09-26 17:10:39 EDT
Upstream does not provide a license file, I've asked them to include one.

In the mean time:
SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.1-3.fc18.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5989432
Comment 6 Terje Røsten 2013-09-26 17:22:54 EDT
Nice, thanks!

A single comment:

diff command creates patches :-) 

Normal naming convention for patches gives:

db2man.sh.diff -> makepasswd-0.5.1-db2man.patch
Makefile.diff  -> makepasswd-0.5.1-makefile.patch

This is clever when your SOURCES/ dir contains lots of patches from
different packages and versions.
Comment 7 Terje Røsten 2013-09-26 17:29:43 EDT
Sorry, two more:

mkdir -p %{buildroot}%{_bindir}
mkdir -p %{buildroot}/%{_mandir}/man1
install -p -m 755 src/makepasswd %{buildroot}%{_bindir}
install -p -m 644 doc/makepasswd.1 %{buildroot}/%{_mandir}/man1

can be done as

install -D -p -m 755 src/makepasswd %{buildroot}%{_bindir}/makepasswd
install -D -p -m 644 doc/makepasswd.1 %{buildroot}/%{_mandir}/man1/makepasswd.1

In more complicated packages than this it's best to write some more than:

 - additional package fixes
 - patch Makefile

write what 'additional package fixes' is and what the Makefile patch *does*.

Doing so will help the reviewer to verify things are fixed, make modifications transparent and also adds trust.
Comment 8 Christopher Meng 2013-09-26 18:35:01 EDT
There is no guideline of writing 1 br per line. 

Just different habits.
Comment 9 Johan Swensson 2013-09-27 01:43:37 EDT
Thanks for the feedback. I will try to better my changelogs from now on. :)

Upstream have now added a license file.

SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.2-1.fc18.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5989943
Comment 10 Johan Swensson 2013-09-27 02:08:12 EDT
Bah, seconds after hitting the save button I noticed a few mistakes. 
Here comes a new one.

SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.2-2.fc18.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5989975
Comment 11 Christopher Meng 2013-09-27 02:10:09 EDT
Hint:

Use make directly now instead of %{__make}.
Comment 12 Terje Røsten 2013-09-27 03:32:26 EDT
%doc macro is clever, drop:

install -D -p -m 644 COPYING %{buildroot}/%{_docdir}/%{name}-%{version}/COPYING
and
%doc %{_docdir}/%{name}-%{version}/COPYING

and use simply in %files:

%doc COPYING
Comment 15 Terje Røsten 2013-10-20 05:04:18 EDT
Package is find, thanks! 

Now you need to find a sponsor.

Please have a look here: 

 https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
Comment 16 Johan Swensson 2013-10-20 14:53:37 EDT
Thanks for the feedback Terje. 

As this is my first submitted review request I'm posting a few links to comments I've done in this request for the formal reviewer/sponsor to have a look at. 

https://bugzilla.redhat.com/show_bug.cgi?id=970407
https://bugzilla.redhat.com/show_bug.cgi?id=1018498
https://bugzilla.redhat.com/show_bug.cgi?id=1019770
https://bugzilla.redhat.com/show_bug.cgi?id=964318
https://bugzilla.redhat.com/show_bug.cgi?id=1013363

And also an update to another package
https://bugzilla.redhat.com/show_bug.cgi?id=988181
Comment 17 Till Maas 2013-10-20 17:31:12 EDT
Please use either $RPM_BUILD_ROOT and $RPM_OPT_FLAGS  or %optflags and %buildroot, but do not mix both:
https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

Also I noticed that %configure adds --Wl,-z,relro to LDFLAGS, but the LDFLAGS in this package do not contain this. I need to check with the devel list, if something needs to be changed here.
Comment 18 Johan Swensson 2013-10-21 02:27:16 EDT
Thank you for taking a look at my request. I have now made changes based on your input and the feedback from devel list.

SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.2-5.fc19.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=6082555

%changelog
* Mon Oct 21 2013 Johan Swensson <kupo@kupo.se> - 0.5.2-5
- Use correct LDFLAGS
- Use optflags macro instead of RPM_OPT_FLAGS
Comment 19 Michael Schwendt 2013-10-21 11:05:46 EDT
> make %{?_smp_mflags} CFLAGSF= CFLAGS="%{optflags}"
> LDFLAGS="%{__global_ldflags} -lcrypt"

Not a blocker, but if you could get upstream to change the src/Makefile to

  LDFLAGS += -lcrypt

it would not be necessary to specify the needed libs in the spec file.


About setting CFLAGS and LDFLAGS manually, also see
https://lists.fedoraproject.org/pipermail/devel/2013-October/190537.html
(particulary, because some reviewers will suggest that in other tickets, too).


> pushd doc
> make

The fedora-review tool here points out that parallel Make invocation is missing. So, I only mention that. If it works for the docs, it could be enabled. Else a comment could tell whether it's missing deliberately or whether it's not considered worthwhile.


> ./docbook.sh -P "/usr/local" -- "makepasswd.1"
> ./docbook.sh -P "/usr/local" -- "makepasswd.html"

Also not an immediate problem, because this $PREFIX parameter does not enter the generated files. But it might become a problem in the future or in similar packages. Paths used during %build ought to match %install.

The fix would be:

  make PREFIX=%{_prefix}
Comment 20 Johan Swensson 2013-10-21 14:52:08 EDT
Thank you for the hints, I will ask upstream to change the Makefile in next release.

SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.2-6.fc19.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=6084683

%changelog
* Mon Oct 21 2013 Johan Swensson <kupo@kupo.se> - 0.5.2-6
- Set correct LDFLAGS automatically
- Added PREFIX 
- Removed unnecessary make step

I kept the CFLAGS override for now because it wasn't passed down to src/Makefile correctly otherwise.
Comment 21 Johan Swensson 2013-10-22 06:44:14 EDT
Sorry about this. I seem to have uploaded the incorrect SRPM and spec with a incorrect changelog.

I have fixed that in this release.

SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.2-7.fc19.src.rpm
Comment 22 Michael Schwendt 2013-10-22 10:13:53 EDT
Let's not turn into a wrong direction. The -7 release breaks setting LDFLAGS. Release -5 was okay, because it passed the wanted LDFLAGS=... to "make". No need to change that.


In -7:

> +%configure || :
> +make %{?_smp_mflags} CFLAGSF= CFLAGS="%{optflags}" LDFLAGS="-lcrypt" PREFIX=%{_prefix}
> 

The %configure macro (see "rpm -E %configure") exports CFLAGS, LDFLAGS (and a few more) env variables, but you override that with the arguments passed to "make".

If you really want to keep that %configure hack and not use %optflags or %__global_ldflags directly, this invocation would work:

  %configure || :
  make %{?_smp_mflags} CFLAGSF= CFLAGS="$CFLAGS" LDFLAGS="$LDFLAGS -lcrypt" PREFIX=%{_prefix}

[...]

The more interesting bit about this package is the included MD5 implementation from RSA:

  https://fedoraproject.org/wiki/Licensing:FAQ#What_about_the_RSA_license_on_their_MD5_implementation.3F_Isn.27t_that_GPL-incompatible.3F

It's an exact copy of the reference implementation found in the RFC (files global.h, md5.h, md5.c). I do not find it on the exceptions list for bundled MD5 implementations:

  https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Packages_granted_exceptions

  https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#cite_note-1

The one called "bundled(md5-polstra)" is close, but that one is only based on the reference impl.and has been edited.

Requesting a bundling exception from the FPC seems to be needed:
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Exceptions


# repoquery --whatprovides bundled\(md5\*|xargs repoquery --provides|grep bundled\(md5|sort|uniq
bundled(md5-deutsch)
bundled(md5-gcc) = 20120403
bundled(md5-gcc) = 20130731
bundled(md5-plumb)
bundled(md5-polstra)

# repoquery --whatprovides bundled\(md5\*|wc -l
16
Comment 23 Johan Swensson 2013-10-22 10:36:13 EDT
Thank you for taking the time to review my package.
My build.log said otherwise about the LDFLAGS, but nevermind. I'm more comfortable with the %__global_ldflags solution in this case if that is acceptable.

I will take a look at requesting an exception.
Comment 24 Michael Schwendt 2013-10-22 11:07:19 EDT
> My build.log said otherwise about the LDFLAGS,

…
cc -o makepasswd makepasswd.o md5c.o  -lcrypt
…

$ rpm -E %__global_ldflags
-Wl,-z,relro 

is missing.

> I'm more comfortable with the %__global_ldflags solution
> in this case if that is acceptable.

Of course!
Comment 25 Johan Swensson 2013-10-23 08:14:13 EDT
Upstream is looking into the possibility to use one of the already approved md5 implementations, would I still need an exception from FPC if one of those were used?
Comment 26 Michael Schwendt 2013-10-23 12:11:30 EDT
No, only for new (or modified) implementations an FPC ticket would need to be opened. The reference implementation has not been covered before. The existing known implementations are treated as "copylib" and have been granted an exception. (see https://fedorahosted.org/fpc/ticket/47 )

The requirements at the packaging level still apply:
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Requirement_if_you_bundle
Comment 27 Johan Swensson 2013-12-20 14:30:04 EST
It now uses openssl's implementation of md5 instead of a bundled one.

SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec
SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.3-1.fc19.src.rpm
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=6320380
Comment 28 Michael Schwendt 2014-01-02 09:46:43 EST
* Indeed, the 25K diff between 0.5.2 and 0.5.3 drops the bundled MD5.


* fedora-review complains about missing parallel make macro, but that's only because of the extra "doc" make target. That's acceptable. Even if the smp flags were dropped from the first make invocation in %build, that would be acceptable IMO, because there's not much to build.


* Related to the "doc" target, a closer look reveals that the top Makefile builds the "doc" subdir target via target "all" already, so the explicit

  pushd doc
  make

in %build is superfluous. rpmdiff suggests that you've readded that step after the -6 spec release.


* Options -e blowfish and -e sha1 are rejected. The man page comments on that, so I only point it out.


* No blockers that would justify requesting another update for review. You may fix the minor "make" issue in dist git.


APPROVED
Comment 29 Johan Swensson 2014-01-02 13:06:36 EST
New Package SCM Request
=======================
Package Name: makepasswd
Short Description: Generates (pseudo-)random passwords of a desired length
Owners: opuk
Branches: f19 f20
Comment 30 Jon Ciesla 2014-01-02 13:11:43 EST
Git done (by process-git-requests).

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