Bug 1012391 - Review Request: makepasswd - Generates (pseudo-)random passwords of a desired length
Summary: Review Request: makepasswd - Generates (pseudo-)random passwords of a desired...
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2013-09-26 11:52 UTC by Johan Swensson
Modified: 2014-01-03 21:04 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2014-01-03 21:04:26 UTC
Type: ---
bugs.michael: fedora-review+
gwync: fedora-cvs+

Attachments (Terms of Use)

Description Johan Swensson 2013-09-26 11:52:41 UTC
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 14:24:46 UTC
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 14:47:58 UTC
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 18:58:18 UTC
> 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


> %{_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.

* 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 19:01:58 UTC
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 21:10:39 UTC
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 21:22:54 UTC
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 21:29:43 UTC
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 22:35:01 UTC
There is no guideline of writing 1 br per line. 

Just different habits.

Comment 9 Johan Swensson 2013-09-27 05:43:37 UTC
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 06:08:12 UTC
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 06:10:09 UTC

Use make directly now instead of %{__make}.

Comment 12 Terje Røsten 2013-09-27 07:32:26 UTC
%doc macro is clever, drop:

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

and use simply in %files:


Comment 15 Terje Røsten 2013-10-20 09:04:18 UTC
Package is find, thanks! 

Now you need to find a sponsor.

Please have a look here: 


Comment 16 Johan Swensson 2013-10-20 18:53:37 UTC
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. 


And also an update to another package

Comment 17 Till Maas 2013-10-20 21:31:12 UTC
Please use either $RPM_BUILD_ROOT and $RPM_OPT_FLAGS  or %optflags and %buildroot, but do not mix both:

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 06:27:16 UTC
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

* 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 15:05:46 UTC
> 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
(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 18:52:08 UTC
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

* 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 10:44:14 UTC
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 14:13:53 UTC
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:


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:



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:

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

# repoquery --whatprovides bundled\(md5\*|wc -l

Comment 23 Johan Swensson 2013-10-22 14:36:13 UTC
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 15:07:19 UTC
> My build.log said otherwise about the LDFLAGS,

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

$ rpm -E %__global_ldflags

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 12:14:13 UTC
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 16:11:30 UTC
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:

Comment 27 Johan Swensson 2013-12-20 19:30:04 UTC
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 14:46:43 UTC
* 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

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.


Comment 29 Johan Swensson 2014-01-02 18:06:36 UTC
New Package SCM Request
Package Name: makepasswd
Short Description: Generates (pseudo-)random passwords of a desired length
Owners: opuk
Branches: f19 f20

Comment 30 Gwyn Ciesla 2014-01-02 18:11:43 UTC
Git done (by process-git-requests).

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