The packaging tempalte from https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file should be used. It's much better to use %pyX_build, etc.

The description should be wrapped to <= 80 columns, this is even visible in the way that bugzilla formats the description above.

Is python fully supported? Than the executables should use python3 not python2.


You can replace the second and subsequent Summary texts with %{summary}, no need to repeat.


Also, don't repeat the %description, it makes the spec file much harder to read than necessary:

%global _description \
XXX \
YYY

%description -n package-name %{_description}

etc.

In %files, please enumerate the items under %{pythonX_sitelib}/ explicitly.
Something like
%{python2_sitelib}/libname
%{python2_sitelib}/libname-%{version}.egg-info

> You can replace the second and subsequent Summary texts with %{summary}, no
> need to repeat.
> 
> 
> Also, don't repeat the %description, it makes the spec file much harder to
> read than necessary:
> 
> %global _description \
> XXX \
> YYY
> 
> %description -n package-name %{_description}
> 
> etc.
> 
> In %files, please enumerate the items under %{pythonX_sitelib}/ explicitly.
> Something like
> %{python2_sitelib}/libname
> %{python2_sitelib}/libname-%{version}.egg-info

All of the above is a matter of opinion and hence the choice of the maintainer.

SPEC: as above
SRPM: https://pbrobinson.fedorapeople.org/python-can-1.5.2-2.fc25.src.rpm

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=16708332

> All of the above is a matter of opinion and hence the choice of the maintainer.

Sure, this was just a suggestion. I do think it makes life easier (certainly for
a reviewer, but I think also for the maintainer).

The license is specified incorrectly: it's LGPLv3+ according to the LICENSE file.

rpmlint:
python2-can.noarch: E: script-without-shebang /usr/bin/j1939_logger.py
python2-can.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/can/util.py /usr/bin/env python3
python2-can.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/can/util.py 644 /usr/bin/env python3
python2-can.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/can/broadcastmanager.py /usr/bin/env python3
python2-can.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/can/broadcastmanager.py 644 /usr/bin/env python3

At least the first should be fixed.

Requires
--------
python2-can (rpmlib, GLIBC filtered):
    /usr/bin/python3 <--------------- this looks wrong
    python(abi)

Provides
--------
python3-can:
    python-can <-------------------- this looks wrong
    python3-can
    python3.5dist(python-can)
    python3dist(python-can)

Oh, it's a typo in python_provide for the py3 subpackage.

Fixed the typo in requires and the license (I think I got that from the PyPi site). I'll report the lint issues upstream.

SPEC: same
SRPM: https://pbrobinson.fedorapeople.org/python-can-1.5.2-3.fc25.src.rpm

License and Provides is fixed, but not Requires and the shebang lines.

python2-can.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/can/broadcastmanager.py /usr/bin/env python3
python2-can.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/can/broadcastmanager.py 644 /usr/bin/env python3
python2-can.noarch: E: script-without-shebang /usr/bin/j1939_logger.py
python2-can.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/can/util.py /usr/bin/env python3
python2-can.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/can/util.py 644 /usr/bin/env python3

Requires
--------
python2-can (rpmlib, GLIBC filtered):
    /usr/bin/python3

(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> License and Provides is fixed, but not Requires and the shebang lines.

The shebang I was going to report upstream and patch once upstream has agreement on it.

> python2-can (rpmlib, GLIBC filtered):
>     /usr/bin/python3

No idea where that comes from  unless one of the binaries are some how specifying it but they don't look to be. Will need to investigate later when I get time.

(In reply to Peter Robinson from comment #7)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> > License and Provides is fixed, but not Requires and the shebang lines.
> 
> The shebang I was going to report upstream and patch once upstream has
> agreement on it.

There are two scripts there:
can_logger.py with "#!/usr/bin/python2 -s",
and
j1939_logger.py without any header.
I don't think there's anything to discuss here, the second one has to be the same as the first.

> > python2-can (rpmlib, GLIBC filtered):
> >     /usr/bin/python3
> 
> No idea where that comes from  unless one of the binaries are some how
> specifying it but they don't look to be. Will need to investigate later when
> I get time.

When building on my F24 machine, the header is as quoted above. When building in rawhide mock, the header is "#!/usr/bin/python3 -s", and that generates the bad dependency in python2-can. Either something changed in rawhide, or it's missing some dependency that influences the header.

I need python-can for another package. It would be nice if we could go on with the review.


BTW, the latest release 3.3.1 [1].


[1] https://github.com/hardbyte/python-can/releases

As of comment #8, there were outstanding dependency issues.
I think it'd be fine to close this one as dead review, and simply open a new review.
Feel free to assign me as reviewer.

... or if Peter wants to pick up the packaging, that'd be great of course too. Sorry,
I shouldn't have discounted this possibility.
tl;dr: I'm happy to finish either this review or a new one, whatever works.

I'll review and update, apologies I missed there was something awaiting me.

Thanks guys.

Updated for review and to latest 3.3.1 upstream release. Dropped the py2 sub-package too.

SPEC: as above
SRPM: https://pbrobinson.fedorapeople.org/python-can-3.3.1-1.fc30.src.rpm
koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=36610367

/usr/lib/python3.7/site-packages/test/ → please either move this under /usr/lib/python3.7/site-packages/can/,
or remove from the package.

rpmlint says:
python3-can.noarch: E: wrong-script-interpreter /usr/lib/python3.7/site-packages/test/data/__init__.py /usr/bin/env python
python3-can.noarch: E: non-executable-script /usr/lib/python3.7/site-packages/test/data/__init__.py 644 /usr/bin/env python
python3-can.noarch: E: wrong-script-interpreter /usr/lib/python3.7/site-packages/test/data/example_data.py /usr/bin/env python
python3-can.noarch: E: non-executable-script /usr/lib/python3.7/site-packages/test/data/example_data.py 644 /usr/bin/env python
... but I don't think it matters much (no dependency is autogenerated). Probably not worth fixing.

+ latest version
+ builds and installs OK
+ package name is OK
+ follows standard python spec
+ P/R/BR look OK
+ license is acceptable (LGPLv3)
+ license is specified correctly

Packages is approved. Please fix that one issue pointed out above.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #15)
> /usr/lib/python3.7/site-packages/test/ → please either move this under
> /usr/lib/python3.7/site-packages/can/,
> or remove from the package.

Will remove.

Thanks

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-can