Bug 1721409

Summary: Review Request: python-pylatex - Library for creating LaTeX files and snippets
Product: [Fedora] Fedora Reporter: Ankur Sinha (FranciscoD) <sanjay.ankur>
Component: Package ReviewAssignee: Zbigniew Jędrzejewski-Szmek <zbyszek>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: package-review, zbyszek
Target Milestone: ---Flags: zbyszek: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2019-06-22 01:03:31 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1276941    

Description Ankur Sinha (FranciscoD) 2019-06-18 08:48:37 UTC
Spec URL: https://ankursinha.fedorapeople.org/python-pylatex/python-pylatex.spec
SRPM URL: https://ankursinha.fedorapeople.org/python-pylatex/python-pylatex-1.3.0-1.fc30.src.rpm

Description: 
PyLaTeX is a Python library for creating and compiling LaTeX files or snippets.
The goal of this library is being an easy, but extensible interface between
Python and LaTeX.


Fedora Account System Username: ankursinha


Rawhide scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=35611005

Comment 1 Zbigniew Jędrzejewski-Szmek 2019-06-18 09:56:51 UTC
My preference is against abbreviations and continuations:
- %global desc %{expand: \
- %description
- %{desc}
+ %global _description %{expand:
+ %description %_description
This will actually remove an unneeded empty line at the beginning of Description.

> PyLaTeX is a Python library for creating and compiling LaTeX files or snippets.
> The goal of this library is being an easy, but extensible interface between
Either add a comma after "extensible", or remove the one before "but". Having
one but not the other is wrong, because it makes encourages the reader to parse
"but extensible interface" as subordinate sentence.

> nosetests-3 tests/*
Are notestests required? pytest is supported better.

> pushd docs || exit -1
-1 is strange. It is the same as 255 in this context, and usually means "killed by a signal". 1 would be better.
But pushd already returns an error, so '|| exit ...' should not be necessary at all.
And in fact, pushd is also unecessary:

make -C docs SPHINXBUILD=sphinx-build-3 html

> BuildRequires:  tex(alltt.sty)
> BuildRequires:  tex(amsmath.sty)
> BuildRequires:  tex(booktabs.sty)
...

That's painful. Hopefully this can be replaced by DynamicBuildrequires in the near future.

+ package name is OK
+ license is acceptable for Fedora (MIT)
+ license is specified correctly
+ builds and installs OK
+ BR/R/P look OK (if a bit tedious)
+ %python_provide macro is present
+ fedora-review and rpmlint are happy

Package is APPROVED.

Comment 2 Zbigniew Jędrzejewski-Szmek 2019-06-18 09:57:38 UTC
%summary: A Python library for creating LaTeX files and snippets → Library for creating LaTeX files and snippets

Comment 3 Ankur Sinha (FranciscoD) 2019-06-18 10:24:00 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> My preference is against abbreviations and continuations:
> - %global desc %{expand: \
> - %description
> - %{desc}
> + %global _description %{expand:
> + %description %_description
> This will actually remove an unneeded empty line at the beginning of
> Description.

Will the subpackage descriptions then similarly become:

%description -n python3-%{pypi_name} %_description

?

> 
> > PyLaTeX is a Python library for creating and compiling LaTeX files or snippets.
> > The goal of this library is being an easy, but extensible interface between
> Either add a comma after "extensible", or remove the one before "but". Having
> one but not the other is wrong, because it makes encourages the reader to
> parse
> "but extensible interface" as subordinate sentence.

Tweaked. (It's upstream's description)

> 
> > nosetests-3 tests/*
> Are notestests required? pytest is supported better.

Upstream does it like this, so I'd like to leave it as it is:
https://github.com/JelteF/PyLaTeX/blob/master/testall.sh#L69

> 
> > pushd docs || exit -1
> -1 is strange. It is the same as 255 in this context, and usually means
> "killed by a signal". 1 would be better.
> But pushd already returns an error, so '|| exit ...' should not be necessary
> at all.
> And in fact, pushd is also unecessary:
> 
> make -C docs SPHINXBUILD=sphinx-build-3 html

Of course! Updated. Still got a pushd in there for the fixes, though.

> 
> > BuildRequires:  tex(alltt.sty)
> > BuildRequires:  tex(amsmath.sty)
> > BuildRequires:  tex(booktabs.sty)
> ...
> 
> That's painful. Hopefully this can be replaced by DynamicBuildrequires in
> the near future.

I had a look at the DynamicBuildRequires page, but I'm not clear how it would pick up all of these tbh. One has to go through the code and figure them out.

> 
> + package name is OK
> + license is acceptable for Fedora (MIT)
> + license is specified correctly
> + builds and installs OK
> + BR/R/P look OK (if a bit tedious)
> + %python_provide macro is present
> + fedora-review and rpmlint are happy
> 
> Package is APPROVED.

Thanks very much! Requesting repository now!

Comment 4 Ankur Sinha (FranciscoD) 2019-06-18 10:25:47 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #2)
> %summary: A Python library for creating LaTeX files and snippets → Library
> for creating LaTeX files and snippets

Updated also.

New spec/srpm:

Spec URL: https://ankursinha.fedorapeople.org/python-pylatex/python-pylatex.spec
SRPM URL: https://ankursinha.fedorapeople.org/python-pylatex/python-pylatex-1.3.0-1.fc30.src.rpm

Comment 5 Ankur Sinha (FranciscoD) 2019-06-18 10:28:29 UTC
https://pagure.io/releng/fedora-scm-requests/issue/12449

Comment 6 Zbigniew Jędrzejewski-Szmek 2019-06-18 12:25:19 UTC
> Will the subpackage descriptions then similarly become:
> %description -n python3-%{pypi_name} %_description
Yep.

> Upstream does it like this, so I'd like to leave it as it is:
> https://github.com/JelteF/PyLaTeX/blob/master/testall.sh#L69
That was just a suggestion. I should have said that. nose is dead, and 
pytest generally gives better reports, so if both work equally well, it's
often better to switch even if upstream has different preferences. But
nosetests is and will be in Fedora of course.

> I had a look at the DynamicBuildRequires page, but I'm not clear how it would pick up all of these tbh. One has to go through the code and figure them out.
Yeah, that's not really possible yet. rpm-4.15 only landed in rawhide,
and we're only really starting to play with it.
In this case there's little hope for an automated tool, but if you
wrote some script to generate the BRs, e.g. by grepping the sources,
the BR generator would have the advantage that it would be able to
always run after %prep, so when upstream inevitably adds new imports,
less packaging work would be required.

Comment 7 Gwyn Ciesla 2019-06-18 13:20:20 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-pylatex

Comment 8 Fedora Update System 2019-06-18 15:49:38 UTC
FEDORA-2019-4d5430f21e has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-4d5430f21e

Comment 9 Fedora Update System 2019-06-19 01:03:11 UTC
python-pylatex-1.3.0-1.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-4d5430f21e

Comment 10 Fedora Update System 2019-06-19 04:15:02 UTC
python-pylatex-1.3.0-1.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-81a7b65fd1

Comment 11 Fedora Update System 2019-06-22 01:03:31 UTC
python-pylatex-1.3.0-1.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.

Comment 12 Fedora Update System 2019-06-28 05:20:52 UTC
python-pylatex-1.3.0-1.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.