Bug 736861 - Review Request: hgview - A fast Mercurial log navigator for qt4 or curses
Summary: Review Request: hgview - A fast Mercurial log navigator for qt4 or curses
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Björn 'besser82' Esser
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-09-08 21:30 UTC by Mads Kiilerich
Modified: 2013-09-12 01:56 UTC (History)
6 users (show)

Fixed In Version: hgview-1.7.1-6.fc19
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-09-12 01:56:47 UTC
besser82: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
improvements for spec-file (5.12 KB, patch)
2013-06-17 11:44 UTC, Björn 'besser82' Esser
no flags Details | Diff

Description Mads Kiilerich 2011-09-08 21:30:39 UTC
Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.3.0-1.fc15.src.rpm
Description:
hgview is a simple tool aiming at visually navigate in a Mercurial repository
history. It is written in Python/Qt4 with quick and efficient key-based
navigation in mind, trying to be fast enough to be usable for big repositories.

$ rpmlint hgview.spec hgview | egrep -v '(spelling-error|incorrect-fsf-address)'
1 packages and 1 specfiles checked; 14 errors, 4 warnings.

Comment 1 Volker Fröhlich 2011-09-08 23:29:03 UTC
Please inform upstream on the wrong address.

Use the name macro in Source0 and the files section.

BuildRequires and Requires are separated by spaces, not commas. Even better: Put each on a separate line.

Drop the version constraint for PyQt4. All possible build targets have a version new enough. The version restriction for Mercurial should probably go to the BRs as well, so the package won't build.

Comment 2 Mads Kiilerich 2011-09-10 09:14:29 UTC
(In reply to comment #1)

Thanks for the quick response.

> Please inform upstream on the wrong address.

https://www.logilab.org/ticket/75295

> Use the name macro in Source0 and the files section.

I don't fully agree with that. I used explicit "hgview" where it didn't refer to the package name or upstream tar name. I don't think this increased use of %{name} increases the readability or flexibility of the spec. But ok ...

> BuildRequires and Requires are separated by spaces, not commas. Even better:
> Put each on a separate line.

Yes, that is another way of doing it, but I don't see any requirement/preference for that in the Packaging Guidelines.

FWIW http://fedoraproject.org/wiki/How_to_create_an_RPM_package and http://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch-specfile-syntax.html describe it as a comma-separated list.

It would be great of the guidelines could help making it more consistent.

> Drop the version constraint for PyQt4. All possible build targets have a
> version new enough.

Ok.

> The version restriction for Mercurial should probably go to
> the BRs as well, so the package won't build.

This build time dependency is bogus and isn't really used, so I assume it would work with all future versions of Mercurial. https://www.logilab.org/ticket/75296


Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.3.0-2.fc15.src.rpm

Comment 4 Volker Fröhlich 2011-10-07 07:24:16 UTC
(In reply to comment #2)

> > Use the name macro in Source0 and the files section.
> 
> I don't fully agree with that. I used explicit "hgview" where it didn't refer
> to the package name or upstream tar name. I don't think this increased use of
> %{name} increases the readability or flexibility of the spec. But ok ...

It depends, I think. The basic rule is to call the package like the tarball. In that case, it fits well. If you're going to rename the package for a different reason, it doesn't help.

> > BuildRequires and Requires are separated by spaces, not commas. Even better:
> > Put each on a separate line.
> 
> Yes, that is another way of doing it, but I don't see any
> requirement/preference for that in the Packaging Guidelines.
> 
> FWIW http://fedoraproject.org/wiki/How_to_create_an_RPM_package and
> http://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch-specfile-syntax.html
> describe it as a comma-separated list.
> 
> It would be great of the guidelines could help making it more consistent.
>

You're right. The reason why I suggested putting them on separate lines is, you can spot duplicates easily. I saw that happen a couple of times, when the list of BRs was long. They're also easier to comment out.

Comment 5 Mads Kiilerich 2011-10-07 08:44:07 UTC
(In reply to comment #4)
> You're right. The reason why I suggested putting them on separate lines is ...

Please try to convince FPC to clarify the packaging guideline - then I would be happy to comply.

Comment 6 Volker Fröhlich 2011-10-07 10:40:36 UTC
There is some typo or grammar mistake in the first line of the description:

"hgview is a simple tool aiming at visually navigate in a Mercurial repository" -- "navigating" maybe?

You should only set python_sitelib for systems that don't define it. Please see http://fedoraproject.org/wiki/Packaging:Python#Macros

The rm in the install section is not necessary. 

Qscintilla-python already requires PyQt4, so you don't have to state that explicitly.

README: "The Text interface depends on urwid, pygments and pyinotify" -- Did you leave that out on purpose?

If you find the time, please correct "intarfece" in the README file and tell upstream. You probably shouldn't ship that file at all, as it gives instructions on how to run it from a check-out, which is not what the package is about.

Comment 7 Mads Kiilerich 2011-11-03 23:33:35 UTC
(In reply to comment #6)

Thanks for the comments. Most of it has just been done. Only a couple of comments:

The description (based on upstreams description) has been changed a lot, so it should perhaps be reviewed again.

> Qscintilla-python already requires PyQt4, so you don't have to state that
> explicitly.

I would like to keep that. It is explicitly stated as a dependency by upstream, and I like to have the important Qt4 dependency explicit.

> README: "The Text interface depends on urwid, pygments and pyinotify" -- Did
> you leave that out on purpose?

I had not updated the package for this new feature. Done now. It is a bit weird that the package both depends on urwid and qt4, but I don't see any good way to split it up. I have however tried not to mix these two parts too much.

Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.4.0-2.fc15.src.rpm

Comment 8 Mads Kiilerich 2012-02-16 22:57:36 UTC
Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.5.0-1.fc16.src.rpm

* Thu Feb 16 2012 Mads Kiilerich <mads@kiilerich.com> - 1.5.0-1
- hgview-1.5.0
- make qt and curses support optional

Comment 9 Mads Kiilerich 2012-11-25 00:06:48 UTC
Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.7.0-1.fc17.src.rpm

* Sat Nov 24 2012 Mads Kiilerich <mads@kiilerich.com> - 1.7.0-1
- hgview-1.7.0

Comment 11 Christopher Meng 2013-05-29 14:52:20 UTC
What about generating the desktop files as source1/2 before building? I seldom see the EOF script in spec now in Fedora.

Comment 12 Mads Kiilerich 2013-05-29 15:33:02 UTC
Yes, that would be another way to do it. I think it would be less readable without being more guideline compliant.

Comment 13 Jason Tibbitts 2013-05-29 18:12:51 UTC
For the record, I agree; there's nothing particularly wrong with catting those simple files out from the spec, and as a bonus you get to use macro substitutions if you want, which you often do.

Unfortunately I know pretty much nothing of mercurial (can barely handle git and don't see the need for yet another VCS of that type) so I don't have any real way to test this but I sure would like to see this finally get reviewed after something like 20 months, so I guess I'll try to take a look.

However, while I can build this just fine, I can't install it because rawhide has mercurial 2.6 and this explicitly requires mercurial < 2.6.  What to do?

Comment 14 Mads Kiilerich 2013-06-05 15:36:00 UTC
(In reply to Jason Tibbitts from comment #13)
> Unfortunately I know pretty much nothing of mercurial (can barely handle git
> and don't see the need for yet another VCS of that type)

Mercurial and git were created simultaneously and independently. For me git is the yet another VCS that I would rather avoid using. YMMV.

> so I don't have any real way to test this 

You can get a repo to browse with
hg clone https://bitbucket.org/logilab/hgview

> but I sure would like to see this finally get reviewed
> after something like 20 months, so I guess I'll try to take a look.

Thanks.

> However, while I can build this just fine, I can't install it because
> rawhide has mercurial 2.6 and this explicitly requires mercurial < 2.6. 
> What to do?

That was a conservative estimate. I have updated the spec with more relaxed requirements.

Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.7.1-2.fc18.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=5471697

Comment 15 Björn 'besser82' Esser 2013-06-09 10:52:28 UTC
duplicate files packaged: `fdupes -r ${_reviewdir}/rpms-unpacked/`

./hgview-curses-1.7.1-2.fc20.noarch.rpm/etc/mercurial/hgrc.d/hgview.rc
./hgview-1.7.1-2.fc20.noarch.rpm/etc/mercurial/hgrc.d/hgview.rc

./hgview-curses-1.7.1-2.fc20.noarch.rpm/usr/share/man/man1/hgview.1.gz
./hgview-1.7.1-2.fc20.noarch.rpm/usr/share/man/man1/hgview.1.gz

./hgview-curses-1.7.1-2.fc20.noarch.rpm/usr/lib/python2.7/site-packages/hgext/hgview.py
./hgview-1.7.1-2.fc20.noarch.rpm/usr/lib/python2.7/site-packages/hgext/hgview.py

./hgview-curses-1.7.1-2.fc20.noarch.rpm/usr/lib/python2.7/site-packages/hgext/hgview.pyc
./hgview-1.7.1-2.fc20.noarch.rpm/usr/lib/python2.7/site-packages/hgext/hgview.pyc

./hgview-curses-1.7.1-2.fc20.noarch.rpm/usr/bin/hgview
./hgview-1.7.1-2.fc20.noarch.rpm/usr/bin/hgview

5 duplicate files (in 5 sets), occupying 8.2 kylobytes

---> A package must NOT list a file more than once in the %files listings.
     see: https://fedoraproject.org/wiki/Packaging/Guidelines#Duplicate_Files

#####

Installation errors
-------------------
INFO: mock.py version 1.1.32 starting...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
Start: run
Mock Version: 1.1.32
INFO: Mock Version: 1.1.32
Start: lock buildroot
INFO: installing package(s): /home/bjoern.esser/fedora/review/736861-hgview/results/hgview-1.7.1-2.fc20.noarch.rpm
ERROR: Command failed: 
 # ['/usr/bin/yum', '--installroot', '/var/lib/mock/fedora-rawhide-x86_64/root/', 'install', '/home/bjoern.esser/fedora/review/736861-hgview/results/hgview-1.7.1-2.fc20.noarch.rpm', '--setopt=tsflags=nocontexts']
Error: Package: hgview-1.7.1-2.fc20.noarch (/hgview-1.7.1-2.fc20.noarch)
            Requires: hgview-common = 1.7.1-2.fc20

---> Why do you split-up the build in three noarched pkgs?
     Having everything in a single noarch-pkg and an additional
     qt-subpkg with desktop-file, icon and Requires: ${qt_stuff} hgview
     would be fine, I suppose?!

#####

cat > $RPM_BUILD_ROOT%{_sysconfdir}/mercurial/hgrc.d/hgview.rc << EOT
[extensions]
# Enable hgview extension to be able to invoke hgview as 'hg hgview' or 'hg qv'.
#hgview = %{python_sitelib}/hgext/hgview.py

---> should be: hgext.hgview = %{python_sitelib}/hgext/hgview.py
     Why do you comment-out by default?

#####

Please fix those sure blockers and I'll run a more detailed review.

Comment 16 Mads Kiilerich 2013-06-10 00:07:28 UTC
(In reply to Björn Esser from comment #15)
> ---> A package must NOT list a file more than once in the %files listings.
>      see: https://fedoraproject.org/wiki/Packaging/Guidelines#Duplicate_Files

You are right. I have seen it used (for instance in grub2) and was sure it was allowed - and don't see why it shouldn't. Including files in multiple sub packages can make it possible to avoid -common packages and avoid providing something before it actually is available.

However, I have moved more files to the -common package.

> Installation errors
...
> Error: Package: hgview-1.7.1-2.fc20.noarch (/hgview-1.7.1-2.fc20.noarch)
>             Requires: hgview-common = 1.7.1-2.fc20

Eeh? Yes, installing a package without its dependencies is an "installation error", not a packaging error.
 
> ---> Why do you split-up the build in three noarched pkgs?
>      Having everything in a single noarch-pkg and an additional
>      qt-subpkg with desktop-file, icon and Requires: ${qt_stuff} hgview
>      would be fine, I suppose?!

There are two independent UIs for hgview. qt users might not want to install the curses dependencies.

> #hgview = %{python_sitelib}/hgext/hgview.py
> 
> ---> should be: hgext.hgview = %{python_sitelib}/hgext/hgview.py

No it should not. hgext is optional (and the left hand side doesn't matter when there is a right hand side).

>      Why do you comment-out by default?

Because it is a best practice for Mercurial that extensions shouldn't be enabled by default. The ability to invoke hgview as 'hg hgview' instead of 'hgview' is also not essential.

Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.7.1-3.fc19.src.rpm

Comment 17 Björn 'besser82' Esser 2013-06-11 12:43:43 UTC
Package has four issues as shown in review-report below.

#####

Package Review
==============

Legend:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://fedoraproject.org/wiki/Packaging:Guidelines

  ---> you should probably just split into two pkgs:
         * hgview (containing former -common and -ncurses)
         * hgview-qt (containing former main-pkg, requiring new-main-pkg)
       ncurses-deps don't occupy that much bandwith and disk-space and
       will make main-pkg basically useful on ANY install.

- Package contains BR: python2-devel or python3-devel
  See: http://fedoraproject.org/wiki/Packaging:Python#BuildRequires

  ---> 's!python-devel!python2-devel!'

- update-desktop-database is invoked when required
  Note: desktop file(s) in hgview
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

  ---> add scriptlets as proposed in link

- Packages should try to preserve timestamps of original installed files.

  ---> -install -m 644 -D %{SOURCE1} ...
       +install -pDm 0644 %{SOURCE1} ...


===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)", "Unknown or generated". 8 files have unknown
     license. Detailed output of licensecheck in
     /home/bjoern.esser/fedora/review/736861-hgview/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

Python:
[x]: Python eggs must not download any dependencies during the build process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[!]: Packages should try to preserve timestamps of original installed files.

     ---> -install -m 644 -D %{SOURCE1} ...
          +install -pDm 0644 %{SOURCE1} ...

[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Spec file according to URL is the same as in SRPM.


Installation errors
-------------------
INFO: mock.py version 1.1.32 starting...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
Start: run
Mock Version: 1.1.32
INFO: Mock Version: 1.1.32
Start: lock buildroot
INFO: installing package(s): /home/bjoern.esser/fedora/review/736861-hgview/results/hgview-1.7.1-3.fc20.noarch.rpm
ERROR: Command failed: 
 # ['/usr/bin/yum', '--installroot', '/var/lib/mock/fedora-rawhide-x86_64/root/', 'install', '/home/bjoern.esser/fedora/review/736861-hgview/results/hgview-1.7.1-3.fc20.noarch.rpm', '--setopt=tsflags=nocontexts']
Error: Package: hgview-1.7.1-3.fc20.noarch (/hgview-1.7.1-3.fc20.noarch)
           Requires: hgview-common = 1.7.1-3.fc20
 You could try using --skip-broken to work around the problem
 You could try running: rpm -Va --nofiles --nodigest



Rpmlint
-------
Checking: hgview-1.7.1-3.fc20.noarch.rpm
hgview.noarch: W: no-documentation
hgview.noarch: W: desktopfile-without-binary /usr/share/applications/hgview.desktop hgview
1 packages and 0 specfiles checked; 0 errors, 2 warnings.




Requires
--------
hgview (rpmlib, GLIBC filtered):
    PyQt4
    hgview-common
    python(abi)
    python-docutils
    qscintilla-python



Provides
--------
hgview:
    hgview



Source checksums
----------------
http://download.logilab.org/pub/hgview/hgview-1.7.1.tar.gz :
  CHECKSUM(SHA256) this package     : 633862c3a2313e5f2432f19b9da9fa19a1ca8f2f2cd0b86df019832e86afc001
  CHECKSUM(SHA256) upstream package : 633862c3a2313e5f2432f19b9da9fa19a1ca8f2f2cd0b86df019832e86afc001


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 736861

#####

Please fix the issues and I'll grant review.

Comment 18 Mads Kiilerich 2013-06-13 01:27:21 UTC
(In reply to Björn Esser from comment #17)

>   ---> you should probably just split into two pkgs:

Thank you for the opinion and advice.

I am still convinced that curses should be a separate package. Installing hgview-curses can take 7.5 MB on disk including the dependencies.

> - Package contains BR: python2-devel

Right. The guidelines has changed since the package was created.

> - update-desktop-database is invoked when required
>   Note: desktop file(s) in hgview
>   See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

The package has no icons - only a pixmap. That section do thus not apply.

You mean http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database ? No - the .desktop file has no MimeType.

> - Packages should try to preserve timestamps of original installed files.
> 
>   ---> -install -m 644 -D %{SOURCE1} ...
>        +install -pDm 0644 %{SOURCE1} ...

That is a file I created by converting the favicon from upstreams website. The "original" timestamp is completely irrelevant. The guidelines says "consider". But ok.

> [!]: Rpmlint is run on all installed packages.
>      Note: Mock build failed
...
> Installation errors
> -------------------
> INFO: mock.py version 1.1.32 starting...
...
> INFO: installing package(s):
> /home/bjoern.esser/fedora/review/736861-hgview/results/hgview-1.7.1-3.fc20.
> noarch.rpm
> ERROR: Command failed: 
>  # ['/usr/bin/yum', '--installroot',
> '/var/lib/mock/fedora-rawhide-x86_64/root/', 'install',
> '/home/bjoern.esser/fedora/review/736861-hgview/results/hgview-1.7.1-3.fc20.
> noarch.rpm', '--setopt=tsflags=nocontexts']
> Error: Package: hgview-1.7.1-3.fc20.noarch (/hgview-1.7.1-3.fc20.noarch)
>            Requires: hgview-common = 1.7.1-3.fc20
>  You could try using --skip-broken to work around the problem
>  You could try running: rpm -Va --nofiles --nodigest

You said that before, and I pointed out that the command you show is attempting do do an invalid installation - that is not a mock build.

Please explain what you are trying to do, what you are doing, and why you think that is the right thing to do.


Spec URL: http://kiilerix.fedorapeople.org/hgview.spec
SRPM URL: http://kiilerix.fedorapeople.org/hgview-1.7.1-4.fc19.src.rpm

Comment 19 Björn 'besser82' Esser 2013-06-17 11:44:29 UTC
Created attachment 762010 [details]
improvements for spec-file

(In reply to Mads Kiilerich from comment #18)
> Thank you for the opinion and advice.
> 
> I am still convinced that curses should be a separate package. Installing
> hgview-curses can take 7.5 MB on disk including the dependencies.

The other way round one can argue about hgview pulling in "half-a-desktop", even if user just needs/wants ncurses interface, e.g. on terminal-only machine.

So I though a bit about it and come in with another aproach making both sides happy, I hope.

When you have a look at the attached patch, you'll see I changed package-order a bit.  There are three pkgs now: hgview (is what was former -common) depending hgview-ui and hgview-{curses,qt} providing hgview-ui, requiring hgview.

This results in 

  a) the user can choose which ui to install

  b) even just installing hgview will install an ui, so we have a working base
     install. Yum should be smart enough to pull in the
     "cheaper = less bandwith/disk" ui.

What's your opinion to this?

Comment 20 Mads Kiilerich 2013-06-17 12:31:59 UTC
(In reply to Björn Esser from comment #19)
> The other way round one can argue about hgview pulling in "half-a-desktop",
> even if user just needs/wants ncurses interface, e.g. on terminal-only
> machine.

If the user wants hgview-curses without the qt stuff in hgview then he just installs hgview-curses.

> What's your opinion to this?

My opinion is that you are proposing changes that might be another valid way to do this packaging but wouldn't improve the packaging. Thanks for sharing your opinion, but it is hardly relevant for a review.

Comment 21 Björn 'besser82' Esser 2013-06-17 13:34:00 UTC
Package is fine.

#####

Package Review
==============

Legend:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- update-desktop-database is invoked when required
  Note: desktop file(s) in hgview
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

  ---> false positive: icon is installed as pixmap.


===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)", "Unknown or generated". 8 files have unknown
     license. Detailed output of licensecheck in
     /home/bjoern.esser/fedora/review/736861-hgview/licensecheck.txt

     ---> License-Tag is fine.

[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

Python:
[x]: Python eggs must not download any dependencies during the build process.
[-]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: hgview-1.7.1-4.fc20.noarch.rpm
hgview.noarch: W: no-documentation
hgview.noarch: W: desktopfile-without-binary /usr/share/applications/hgview.desktop hgview
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

---> false positve: "binary" gets pulled by dependency.


Rpmlint (installed packages)
----------------------------
# rpmlint hgview
hgview.noarch: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
hgview (rpmlib, GLIBC filtered):
    PyQt4
    hgview-common
    python(abi)
    python-docutils
    qscintilla-python



Provides
--------
hgview:
    hgview



Source checksums
----------------
http://download.logilab.org/pub/hgview/hgview-1.7.1.tar.gz :
  CHECKSUM(SHA256) this package     : 633862c3a2313e5f2432f19b9da9fa19a1ca8f2f2cd0b86df019832e86afc001
  CHECKSUM(SHA256) upstream package : 633862c3a2313e5f2432f19b9da9fa19a1ca8f2f2cd0b86df019832e86afc001


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 736861

#####

APPROVED!

Comment 22 Michael Schwendt 2013-06-21 18:19:16 UTC
> %files
> %{python_sitelib}/hgviewlib/qt4/

> %files -n %{name}-common
> %{python_sitelib}/hgext/%{name}.py*

%python_sitelib is below /usr/lib for x86_64, whereas mercurial.x86_64 stores its extensions below /usr/lib64 => %python_sitearch:
http://koji.fedoraproject.org/koji/packageinfo?packageID=2518

It does not include %{python_sitelib}/hgext/ yet, but some package ought to own it: https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

Comment 23 Mads Kiilerich 2013-06-21 19:00:52 UTC
(In reply to Michael Schwendt from comment #22)

Hmm ... yeah ... nothing owns /usr/lib/.../hgext on x86_64 ... but it is owned on x86.

Mercurial on x86_64 doesn't look in /usr/lib/.../hgext at all so it would be strange if Mercurial owned that directory.

For almost the same reasons my sample hg config for this extension uses an absolute path. This hgview.py should perhaps just be stored in the ordinary hgview directory instead of in an hgext directory.

Comment 24 Christopher Meng 2013-07-22 03:26:13 UTC
NEWS?

Comment 25 Mads Kiilerich 2013-09-01 11:47:29 UTC
New Package SCM Request
=======================
Package Name: hgview
Short Description: A fast Mercurial log navigator for qt4 or curses
Owners: kiilerix
Branches: f19 f20
InitialCC:

Comment 26 Kevin Fenzi 2013-09-01 18:26:13 UTC
Git done (by process-git-requests).

Comment 27 Fedora Update System 2013-09-01 20:01:20 UTC
hgview-1.7.1-6.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/hgview-1.7.1-6.fc19

Comment 28 Fedora Update System 2013-09-02 23:26:27 UTC
Package hgview-1.7.1-6.fc19:
* should fix your issue,
* was pushed to the Fedora 19 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing hgview-1.7.1-6.fc19'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-15644/hgview-1.7.1-6.fc19
then log in and leave karma (feedback).

Comment 29 Fedora Update System 2013-09-12 01:56:47 UTC
hgview-1.7.1-6.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.


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