Bug 997780 - Review Request: gumbo-parser - A HTML5 parser library
Review Request: gumbo-parser - A HTML5 parser library
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-16 04:24 EDT by Ralf Corsepius
Modified: 2013-11-10 01:57 EST (History)
3 users (show)

See Also:
Fixed In Version: gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-10-26 23:56:09 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
review.txt (10.26 KB, text/plain)
2013-10-15 02:28 EDT, Remi Collet
no flags Details

  None (edit)
Description Ralf Corsepius 2013-08-16 04:24:58 EDT
Spec URL: http://corsepiu.fedorapeople.org/packages/gumbo-parser.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/gumbo-parser-1.0-1.20130816git88ee911.fc20.src.rpm
Description: 
Gumbo is an implementation of the HTML5 parsing algorithm implemented as
a pure C99 library with no outside dependencies. It's designed to serve
as a building block for other tools and libraries such as linters,
validators, templating languages, and refactoring and analysis tools.

Fedora Account System Username: corsepiu
Comment 1 Remi Collet 2013-10-15 02:28:37 EDT
Created attachment 812353 [details]
review.txt

Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -b 997780
Buildroot used: fedora-19-x86_64
Active plugins: Python, Generic, Shell-api, C/C++
Disabled plugins: Java, SugarActivity, Perl, R, PHP, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG
Comment 2 Remi Collet 2013-10-15 02:36:40 EDT
Kohi scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6061098

MUST 

[!]: Package meets the Packaging Guidelines::Python
        From recent Guidelines change:
        Package don't contains BR: python2-devel or python3-devel
        The unversioned macro, %{__python} is deprecated.
        use {__python2} %{python2_sitelib} macros to be consistent

        Notice, I will prefer the python BR in python sub-package.

[!]: Package is named according to the Package Naming Guidelines.
        Version 1.0 is not released, so this is a pre-release
        => Release: 0.1....


[!]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[!]: If (and only if) the source package includes the text of the license(s)

      "Common licenses that require including their texts with all derivative works include ASL 2.0..."

        => COPYING is now present on github

SHOULD

[!]: Package consistently uses macros (instead of hard-coded directory names).
        => Could use %{name} in URL, python description, ...

COULD

[!]: Too large wildcard, because I dislike them ;)
        %{_libdir}/libgumbo.so.1*
        %{python_sitelib}/gumbo*
Comment 3 Remi Collet 2013-10-15 02:42:04 EDT
Can you please check why _GumboNode.3 instead of GumboNode.3 ?
(exactly why I dislike wide wildcard)
Comment 4 Ralf Corsepius 2013-10-17 04:55:12 EDT
(In reply to Remi Collet from comment #2)
> Kohi scratch build:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=6061098
> 
> MUST 
> 
> [!]: Package meets the Packaging Guidelines::Python
>         From recent Guidelines change:
>         Package don't contains BR: python2-devel or python3-devel
>         The unversioned macro, %{__python} is deprecated.
>         use {__python2} %{python2_sitelib} macros to be consistent

Well, as you know, I consider this FPC decision to be a non-helpful mistake, exactly because of cases like these.

- The python bindings of this package are optional.
- The package's python bindings are not tied to any particular version of python.
- The package is supposed to be build against the distribution's "default python".

=> Enforcing to use python2|3 means unnecessarily tying the package against a specific version of python.

That said, thanks to the churn this all causes, I am quite strongly leaning towards not packaging the python bindings at all.

For now, I have decided to unconditionally switch to python3, because I do not see much sense in adding support for an discontinued version of python in a new package.

>         Notice, I will prefer the python BR in python sub-package.
I don't understand.

> [!]: Package is named according to the Package Naming Guidelines.
>         Version 1.0 is not released, so this is a pre-release
>         => Release: 0.1....
Well, upstream is quite inconsistent on this. Internally, they consistently use version 1.0. In README.md, they are talking about 0.9.

As the Release-tag is not of much importance (and doesn't make any difference to users) I an switching to using Release: 0.x.y
 
> [!]: If the source package does not include license text(s) as a separate
> file
>      from upstream, the packager SHOULD query upstream to include it.
> [!]: If (and only if) the source package includes the text of the license(s)
> 
>       "Common licenses that require including their texts with all
> derivative works include ASL 2.0..."
> 
>         => COPYING is now present on github
Yes. Wasn't present at the time, I pulled the tarball.

> SHOULD
> 
> [!]: Package consistently uses macros (instead of hard-coded directory
> names).
>         => Could use %{name} in URL, python description, ...

Done.
 
> COULD
> 
> [!]: Too large wildcard, because I dislike them ;)
>         %{_libdir}/libgumbo.so.1*
>         %{python_sitelib}/gumbo*
My personal preference differs.

(In reply to Remi Collet from comment #3)
> Can you please check why _GumboNode.3 instead of GumboNode.3 ?
Upstream bug. They fixed it.


Updated package with a couple of more issues fixed:

Spec URL: http://corsepiu.fedorapeople.org/packages/gumbo-parser.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc21.src.rpm
Comment 5 Remi Collet 2013-10-17 05:15:19 EDT
I mostly agree on those python macro... but "we" have approve this ;)

>>         Notice, I will prefer the python BR in python sub-package.
>I don't understand.

I just mean to move the "BuildRequires: python3-setuptools python3-devel" in the "%package python" section of the spec. But probably it only make sense to me ;)


[x]: Package meets the Packaging Guidelines::Python
[x]: Package is named according to the Package Naming Guidelines.
[x]: license text(s) 
[x]: Package consistently uses macros (instead of hard-coded directory names).


So: APPROVED
Comment 6 Ralf Corsepius 2013-10-17 08:00:01 EDT
New Package SCM Request
=======================
Package Name: gumbo-parser
Short Description: A HTML5 parser library
Owners: corsepiu
Branches: f18 f19 f20
Comment 7 Ralf Corsepius 2013-10-17 08:13:58 EDT
First of all thanks for the review!

(In reply to Remi Collet from comment #5)
> I mostly agree on those python macro... but "we" have approve this ;)

Likely we are going to see if/else %fedora/%rhel cascades in rpm-specs to switch to the different "default python" because %__python was banned.

IMNSHO, this is stupid and needs to be revisited.

> >>         Notice, I will prefer the python BR in python sub-package.
> >I don't understand.
> 
> I just mean to move the "BuildRequires: python3-setuptools python3-devel" in
> the "%package python" section of the spec. But probably it only make sense
> to me ;)
:=) AFAICT, it technically doesn't matter all.
Comment 8 Gwyn Ciesla 2013-10-17 08:23:16 EDT
Git done (by process-git-requests).
Comment 9 Fedora Update System 2013-10-17 11:48:11 EDT
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc19
Comment 10 Fedora Update System 2013-10-17 11:48:23 EDT
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc18
Comment 11 Fedora Update System 2013-10-17 11:48:38 EDT
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc20
Comment 12 Fedora Update System 2013-10-17 16:33:21 EDT
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc20 has been pushed to the Fedora 20 testing repository.
Comment 13 Fedora Update System 2013-10-26 23:56:09 EDT
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc19 has been pushed to the Fedora 19 stable repository.
Comment 14 Fedora Update System 2013-10-27 01:31:44 EDT
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc19 has been pushed to the Fedora 19 stable repository.
Comment 15 Fedora Update System 2013-10-27 01:38:31 EDT
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc18 has been pushed to the Fedora 18 stable repository.
Comment 16 Fedora Update System 2013-11-10 01:57:09 EST
gumbo-parser-1.0-0.2.20131001gitd90ea2b.fc20 has been pushed to the Fedora 20 stable repository.

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