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.
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.
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
> 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> 0.5.1-2 - package fixes with input from informal review Insert empty line here. * Mon Sep 26 2013 Johan Swensson <kupo> 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.
One more: warning: bogus date in %changelog: Mon Sep 26 2013 Johan Swensson <kupo> 0.5.1-1 Sep 26 20113 is Thu, not Mon.
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
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.
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.
There is no guideline of writing 1 br per line. Just different habits.
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
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
Hint: Use make directly now instead of %{__make}.
%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
SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.2-3.fc18.src.rpm Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5990127
SPEC: http://kupo.se/pub/makepasswd/makepasswd.spec SRPM: http://kupo.se/pub/makepasswd/makepasswd-0.5.2-4.fc19.src.rpm Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=6080594 Minor update just correcting the changelog.
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
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
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.
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> - 0.5.2-5 - Use correct LDFLAGS - Use optflags macro instead of RPM_OPT_FLAGS
> 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}
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> - 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.
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
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
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.
> 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!
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?
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
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
* 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
New Package SCM Request ======================= Package Name: makepasswd Short Description: Generates (pseudo-)random passwords of a desired length Owners: opuk Branches: f19 f20
Git done (by process-git-requests).