Bug 985051 - Review Request: vcprompt - efficient program to print VCS info on your prompt
Review Request: vcprompt - efficient program to print VCS info on your prompt
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2013-07-16 12:34 EDT by steven.merrill
Modified: 2015-02-27 12:18 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description steven.merrill 2013-07-16 12:34:39 EDT
Spec URL: https://raw.github.com/smerrill/vcprompt-spec/master/vcprompt.spec
SRPM URL: http://packages.fayze2.com/vcprompt-1.1-1.fc19.src.rpm
Description: vcprompt is an efficient C program to print VCS information to your prompt. The spec file passes RPM lint and a Koji scratch build succeeded: http://koji.fedoraproject.org/koji/taskinfo?taskID=5613491 .
Fedora Account System Username: stevenmerrill
Comment 1 steven.merrill 2013-07-16 12:39:37 EDT
Apologies. Please find the SRPM at https://github.com/smerrill/vcprompt-spec/raw/master/vcprompt-1.1-1.fc19.src.rpm .
Comment 2 Mario Blättermann 2013-07-16 14:03:07 EDT
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.
Comment 3 Mario Blättermann 2013-07-16 14:20:00 EDT
As far as I can see, you are not a member of the package maintainers group. Adding FE-NEEDSPONSOR.
Comment 4 steven.merrill 2013-07-16 15:08:03 EDT
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.
Comment 5 Michael Schwendt 2014-01-22 06:35:05 EST
> 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.
Comment 6 Michael Schwendt 2015-01-24 10:37:59 EST
Steven,

having noticed your brand-new review request bug 1185550, do you have comments on this particular one?
Comment 7 steven.merrill 2015-01-24 14:01:28 EST
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!
Comment 8 Michael Schwendt 2015-02-27 12:18:10 EST
> 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?

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