Bug 985051

Apologies. Please find the SRPM at https://github.com/smerrill/vcprompt-spec/raw/master/vcprompt-1.1-1.fc19.src.rpm .

The C compiler and make are part of the minimum build environment:
http://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
It is unneeded to mention them in BuildRequires.

PREFIX=/usr has to be replaced with PREFIX=%{prefix}. Don't use such hardcoded paths. This also applies to the file list, we have macros for all system folders (%{_bindir}, %{_datadir}, %{_mandir} etc.).

Don't use a certain file extension for manpages. Well, currently they are gzipped, but this could change in the future. Use an asterisk * instead:
%{_mandir}/man1/vcprompt.1.*

BTW, the man page is not directly to be added to %doc. But I'm missing README.txt here.

rm -rf $RPM_BUILD_ROOT
is an artifact from older Fedora releases. The current behavior defaults to that, so it can be safely dropped.

As far as I can see, you are not a member of the package maintainers group. Adding FE-NEEDSPONSOR.

Mario,

Thank you for the prompt response and the feedback. Here are updated files:

SPEC: https://raw.github.com/smerrill/vcprompt-spec/master/vcprompt.spec
SRPM: https://github.com/smerrill/vcprompt-spec/raw/master/vcprompt-1.1-1.fc19.src.rpm

Here is the overall diff with the incorporated feedback: https://github.com/smerrill/vcprompt-spec/compare/ccba2bf67ce3abb337d01059a9ae6937d54a390b...master#diff-1

A Koji scratch build for the updated spec is online at http://koji.fedoraproject.org/koji/taskinfo?taskID=5615258.

> PREFIX=/usr has to be replaced with PREFIX=%{prefix}.

Sorry for disagreeing here, but the guidelines are more lax nowadays:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Macros

A hardcoded /usr for this tiny package would not be a blocker. Since the package has been changed already, that's good. One can actually rebuild it with "--define _prefix /foo", but the benefit is small.

[...]

> Summary:        An efficient program to print VCS information on your prompt

Even more consise, IMO:

  Summary: Print VCS information on your prompt


> Source0:        http://hg.gerg.ca/vcprompt/archive/%{version}.tar.gz

It would be great, if upstream could add the name "vcprompt" to the source tarball. Currently, it's just "1.1.tar.gz", which may be considered enough as part of a full URL, but on local storage it's too short and confusing.


> make %{?_smp_mflags}

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

This "make" invocation in %build ought to set PREFIX=%{_prefix} and MANDIR=%{_mandir}/man1, too, just in case the build process substitutes those values into any built file (such as inline help or documentation). That's a general hint for any package. For many packages there won't be any issues, but there are exceptions.


> $ rpmls -p vcprompt-1.1-1.fc20.x86_64.rpm 
> -rwxr-xr-x  /usr/bin/vcprompt
> -rwxr-xr-x  /usr/man/man1/vcprompt.1.gz
> drwxr-xr-x  /usr/share/doc/vcprompt
> -rw-r--r--  /usr/share/doc/vcprompt/README.txt

The manual page should not be executable.

The path for the manual page is wrong, but only because the src.rpm includes an old spec file that's missing some fixes.

If you keep the lines with "Spec URL:" and "SRPM URL:" in this ticket up-to-date, you could point the fedora-review tool at this ticket: fedora-review -b 985051

Running rpmlint (or "rpmlint -i …") on the src.rpm and all built rpms can be helpful and is a requirement by the review guidelines, too:

  vcprompt.x86_64: W: spurious-executable-perm /usr/man/man1/vcprompt.1.gz
  vcprompt.x86_64: W: manual-page-warning /usr/man/man1/vcprompt.1.gz 284: a space character is not allowed in an escape name
  vcprompt.x86_64: W: non-standard-dir-in-usr man


> Makefile

It includes a "check" target, which executes fine here, but skips some tests. Do you think all tests in that test-suite is suiteable for a %check section? Then add

  %check
  make check

after the %install section. It may require additional BuildRequires for some of the tests. If any of the tests require network access, that won't be possible in the Fedora Build System, however.

Steven,

having noticed your brand-new review request bug 1185550, do you have comments on this particular one?

Spec URL: https://raw.githubusercontent.com/smerrill/vcprompt-spec/master/SPECS/vcprompt.spec
SRPM URL: https://raw.githubusercontent.com/smerrill/vcprompt-spec/master/SRPMS/vcprompt-1.2.1-1.fc21.src.rpm

Michael, thanks for the ping on this. I have updated to v1.2.1, and I think I've incorporated all the feedback. Upstream's Makefile now supports DESTDIR, which let me cut out %make_install.

The full diff in the specs since last time is here: http://fpaste.org/174040/22125836/

Now rpmlint -i only has two complaints:

$ rpmlint -i SPECS/* RPMS/*/* SRPMS/*

SPECS/vcprompt.spec: W: invalid-url Source0: https://bitbucket.org/gward/vcprompt/downloads/vcprompt-1.2.1.tar.gz HTTP Error 403: Forbidden
The value should be a valid, public HTTP, HTTPS, or FTP URL.

vcprompt.src: W: invalid-url Source0: https://bitbucket.org/gward/vcprompt/downloads/vcprompt-1.2.1.tar.gz HTTP Error 403: Forbidden
The value should be a valid, public HTTP, HTTPS, or FTP URL.

3 packages and 1 specfiles checked; 0 errors, 2 warnings.

These two warnings are happening because BitBucket redirects requests for their tarball downloads to an S3 URL that is only valid for a short time. It is preferable to use the BitBucket downloads for two reasons:

1) The tarballs now have vcprompt in their names, which was requested earlier in this thread.
2) The downloads from BitBucket come with a configure script so that autoconf does not have to be run. (I read that autoconf use was discouraged in http://fedoraproject.org/wiki/PackagingDrafts/AutoConf.)

Let me know what you think!

> http://fedoraproject.org/wiki/PackagingDrafts/AutoConf

That's just an old "draft" - an opinion piece - not a final guideline approved by the FPC. Also notice the comment someone has left at the top. ;-)

Fact is, regenerating Autotools files is not trivial for all projects. At best, they would fail to rebuild and you would get error messages. At worst, they are broken silently (and e.g. you lose macros or inserted/appended modifications which result in building the source code differently and possibly in a wrong way).


> Upstream's Makefile now supports DESTDIR, which let me cut out %make_install.

%make_install is one of the macros that's _recommended_. It differs from %makeinstall (no '_' in there!). See:  rpm -E %make_install

  https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

Currently, it would only save some typing compared with your full invocation of "make install ...". Good to know, but that's all.


> make %{?_smp_mflags}
> 
> %install
> make install DESTDIR=%{buildroot} PREFIX=%{_prefix} MANDIR=%{buildroot}%{_mandir}/man1

Noticing the extra %buildroot here in the MANDIR definition, I've had a look at "Makefile*". It's a mix between a manually written Makefile and one that uses Autotools. It contains some questionable definitions for PREFIX, BINDIR, MANDIR and DESTDIR, which should not be done like that.

After running the configure script, it should not be necessary anymore to override PREFIX or MANDIR anymore when running "make". Typically, "configure" defines those paths already and substitutes them in the generated Makefiles. See "rpm -E %configure" for the options passed to the configure script. It would be good to use what "configure" determines, so there are no competing and potentially conflicting places where paths (or other values) are defined. Autotools even generate install paths based on the values it determines (e.g. $prefix, $bindir, $mandir).


> %build
> %configure

This contains a check for SQLite 3, and indeed ./src/svn.c contains conditional code that uses SQLite. It's a bit fragile, because it doesn't error out if SQLite 3 is not found. If you want to add BuildRequires for it, you may want to add a safety check that verifies that SQLite 3 is found and built with.

Else build with --with-sqlite3=no to disable that feature explicitly.


> %check
> make check

Any comment on what I asked about at the bottom of comment 5?

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.