Bug 2003700 - Review Request: container-workflow-tool - Tool for automation of rebuilding container images
Summary: Review Request: container-workflow-tool - Tool for automation of rebuilding ...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Michal Schorm
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-09-13 13:21 UTC by Zuzana Miklankova
Modified: 2021-11-03 11:10 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-11-03 11:10:49 UTC
Type: ---
Embargoed:
mschorm: fedora-review+


Attachments (Terms of Use)

Description Zuzana Miklankova 2021-09-13 13:21:07 UTC
Hello, this is my first attempt for packing a fedora package.

Spec URL: https://copr-dist-git.fedorainfracloud.org/cgit/zmiklank/container-workflow-tool/container-workflow-tool.git/plain/container-workflow-tool.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/zmiklank/container-workflow-tool/srpm-builds/02725716/container-workflow-tool-1.0.0-1.fc34.src.rpm
Description: A python3 library to make rebuilding container images easier by automating several steps of the process.
Fedora Account System Username: zmiklank


The koji build can be found here: https://koji.fedoraproject.org/koji/taskinfo?taskID=75603102

Comment 1 Michal Schorm 2021-09-14 12:01:16 UTC
I will be the Reviewer of this package.

Comment 2 Michal Schorm 2021-09-14 12:07:45 UTC
The author wants to get sponsored.
Setting the information for her.

Comment 3 Michal Schorm 2021-09-14 12:48:02 UTC
Please use plain links to the files them selves, such as:

Spec URL: https://copr-dist-git.fedorainfracloud.org/cgit/zmiklank/container-workflow-tool/container-workflow-tool.git/plain/container-workflow-tool.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/zmiklank/container-workflow-tool/srpm-builds/02725716/container-workflow-tool-1.0.0-1.fc34.src.rpm

Otherwise various tools don't work, as they cannot download the files, and get some html file rather then the spec file.

Comment 4 Michal Schorm 2021-09-17 00:51:34 UTC
I've went through the "Python Packaging Guidelines" chapter of "Fedora Packaging Guidelines" the best I could.
However I require another packager to go through my findings and add a second opinion,
since this is just the second Python package ever I dive into.
I need the second pair of eyes.

https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/

---

1/ BuildRequire python3-devel [NEED-2ND-OPINION]
[NEED-2ND-OPINION] "Every package that uses Python (at runtime and/or build time) and/or installs Python modules
                   MUST explicitly include BuildRequires: python3-devel in its .spec file,
                   even if Python is not actually invoked during build time."
Reviewer note:
  Your code:
    BuildRequires:  python%{python3_pkgversion}-devel
  I believe the Guideline explicitly shows the code that is mandatory to be included. And it is without macros.
  _My understanding_ is that the pure following line MUST be used instead:
    BuildRequires: python3-devel


2/ Naming [OK]
Package name:
  container-workflow-tool
Canonical name:
  | rpm --eval "%{py_dist_name container-workflow-tool}"
  | container-workflow-tool
Main importable module name:
  container_workflow_tool

This package provides BOTH a tool (application) and the importable module.
Currently the primarly function as I see it is to provide the application, so the following apply:
  "Packages that primarily provide applications, services or any kind of executables
  SHOULD be named according to the general Fedora naming guidelines (e.g. ansible)."

Also:
  "Consider adding a virtual provide according to Library naming above
  (e.g. python3-PROJECTNAME), if it would help users find the package."
Since the goal is _also_ to provide the importable module, it might be a good idea to add such virtual provide.


3/ Files to include [OK]

[OK] "Packages MUST include the source file (*.py) AND
     the bytecode cache (*.pyc) for each pure-Python importable module."
[OK] "The source files MUST be included in the same package as the bytecode cache."
[OK] "Scripts that are not importable (typically ones in %{_bindir} or %{_libexecdir})
     SHOULD NOT be byte-compiled."
[OK] "The cache files are found in a __pycache__ directory and
     have an interpreter-dependent suffix like .cpython-39.pyc."


4/ Dist-info metadata [OK]

[OK] "Each Python package MUST include Package Distribution Metadata conforming
     to PyPA specifications (specifically, Recording installed distributons)."
[OK] "The metadata SHOULD be included in the same subpackage as
     the main importable module, if there is one."


5/ Explicit lists [FAIL]
[OK] "Packages MUST NOT own shared directories owned by Python itself"
[FAIL] "Packagers SHOULD NOT simply glob everything under a shared directory."
Reviewer note:
  the package ship a single manual page. Please list the file explicitly, instead of:
  |  %{_mandir}/man1/*


6/ PyPI parity [FAIL]
[FAIL] "Every Python package in Fedora SHOULD also be available on the Python Package Index (PyPI)."
Reviewer note:
  The attempt to make the project available via PyPI has yet not been made.
[OK] "The command pip install PROJECTNAME MUST install the same package
     (possibly in a different version), install nothing, or fail with a reasonable error message."
[FAIL] "If your package is not or cannot be published on PyPI, you can:
       * Ask Python SIG to block the name on PyPI for you"
Reviewer note:
  It is unclear whether the upstream would like to maintain the project in PyPI,
  however to avoid future name collisions, it seems wise to me to block the name.
  If the name collision would happen, you (maintainer) would be force to change the package name AND
  you would have to try to convince upstream to rename its projects, since the names of the
  upstream project and the Fedora package SHOULD match.
  That is my understanding of the Python Naming Guidelines


7/ Provides and requirements [FAIL]
|    Requires
|    --------
|    container-workflow-tool (rpmlib, GLIBC filtered):
|        /usr/bin/python3
|        python3-container-workflow-tool
|    python3-container-workflow-tool (rpmlib, GLIBC filtered):
|        python(abi)
|        python3.10dist(gitpython)
|        python3.10dist(pyyaml)
|        python3.10dist(requests-kerberos)
|    Provides
|    --------
|    container-workflow-tool:
|        container-workflow-tool
|    python3-container-workflow-tool:
|        python-container-workflow-tool
|        python3-container-workflow-tool
|        python3.10-container-workflow-tool
|        python3.10dist(container-workflow-tool)
|        python3dist(container-workflow-tool)
[OK] "For any module intended to be used in Python 3 with import MODNAME,
     the package that includes it SHOULD provide python3-MODNAME,
     with underscores (_) replaced by dashes (-)."
[OK] "For any FOO, a package that provides python3-FOO SHOULD use %py_provides
     or an automatic generator to also provide python-FOO and python3.X-FOO,
     where X is the minor version of the interpreter."
[OK] "The provide SHOULD NOT be added manually: if a generator or macro is not used,
     do not add the python-FOO / python3.X-FOO provides at all."
[OK] "Every Python package MUST provide python3dist(DISTNAME) and python3.Xdist(DISTNAME),
     where X is the minor version of the interpreter and
     DISTNAME is the Canonical project name corresponding to the Dist-info metadata."
[OK] "This is generated automatically from the dist-info metadata.
     The provide SHOULD NOT be added manually: if the generator fails to add it, the metadata MUST be fixed."
[FAIL] "Each Python package MUST explicitly BuildRequire python3-devel."
Reviewer note:
  "BuildRequires:  python%{python3_pkgversion}-devel" is used instead.
[OK] "Packages MUST NOT have dependencies (either build-time or runtime)
     with the unversioned prefix python- if the corresponding python3- dependency can be used instead."
[OK] "Packages SHOULD NOT have explicit dependencies (either build-time or runtime)
     with a minor-version prefix such as python3.8- or python3.8dist("
[OK] "Packages SHOULD NOT have an explicit runtime dependency on python3."
[NEED-2ND-OPINION] "Packages MUST use the automatic Python run-time dependency generator."
[OK] "Packages SHOULD use the opt-in build-dependency generator if possible."
[OK] "The packager MUST inspect the generated requires for correctness.
     All dependencies MUST be resolvable within the targeted Fedora version."
[OK] "Dependencies covered by the generators SHOULD NOT be repeated in the .spec file."
[NEED-2ND-OPINION] The project README.md states that:
  |  Requirements
  |  ------------
  |  * python3
  |  * python3-GitPython
  |  * python3-requests-kerberos
  |  * fedpkg
  |  * http://github.com/ficap/dhwebapi (installed using pip3 and requirements.txt)
  But I see those requirements neither generated nor explicitly specified.
  Shouldn't they be listed?


8/ Interpreter invocation [NEED-2ND-OPINION]
[OK] "Shebang lines to invoke Python MUST use %{python3} as the interpreter."
[OK] "Shebang lines to invoke Python SHOULD be #!%{python3} -%{py3_shebang_flags} and they MAY include extra flags."
Reviewer note:
  macro "%pyproject_install" is used.
  | # grep -i -e "^#!" -r review-container-workflow-tool/rpms-unpacked/*
  | review-container-workflow-tool/rpms-unpacked/container-workflow-tool-1.0.0-1.fc36.noarch.rpm/usr/bin/cwt:#! /usr/bin/python3 -s
[NEED-2ND-OPINION] "Every executable TOOL for which the current version of Python matters
                   SHOULD also be invokable by python3 -m TOOL."
                   "If the software doesn’t provide this functionality, packagers SHOULD ask the upstream to add it."
Reviewer note:
  I don't know how to check if the current version of Python matters.


9/ Using Cython [OK]
[OK] "packages MUST NOT use files pre-generated by Cython"


10/ Tests [FAIL]
[FAIL] "If a test suite exists upstream, it SHOULD be run in the %check section.
       If that is not possible with reasonable effort,
       at least a basic smoke test (such as importing the packaged module) MUST be run in %check."
Reviewer note:
  No test is currently run.
  Even thought the justification why the upstream test are not run is OK, it does NOT follow the guidelines.
  You MUST run atleast a basic smoke test.
[OK] "In %check, packages SHOULD NOT run “linters”: code style checkers,
     test coverage checkers and other tools that check code quality rather than functionality."


11/ Source files from PyPI [OK]
Reviewer note:
  None used.

Comment 5 Michal Schorm 2021-09-17 01:05:34 UTC
Fedora Packaging Guidelines (NOT Python specific) findings:


11/ Man page installation
Please consider using an alternative approach of packing the man page.
Go through:
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
And take a look if the '%_pkgdocdir' macro won't help you.
If nothing in that section would enhance your approach, I'm fine with the current code.


12/ LICENSE file
I do not see any usage of '%license' macro in the '%files' section, however
the License file is present in the package:
  python3-container-workflow-tool-1.0.0-1.fc36.noarch.rpm/usr/lib/python3.10/site-packages/container_workflow_tool-1.0.0.dist-info/LICENSE


13/ Configuration files
I can see the YAML config files in the

| python3-container-workflow-tool-1.0.0-1.fc36.noarch.rpm
| └── usr
|     └── lib
|         └── python3.10
|             └── site-packages
|                 └── container_workflow_tool
|                     ├── cli_common.py
|                     ├── cli.py
|                     └── config
|                         ├── default.yaml
|                         ├── f27.yaml
|                         ├── f28.yaml
|                         ├── f29.yaml
|                         ├── f30.yaml
|                         ├── f31.yaml
|                         ├── f32.yaml
|                         ├── f33.yaml
|                         ├── f34.yaml
|                         ├── rawhide.yaml
|                         └── share
|                             ├── cwt_config.yaml
|                             └── urls.yaml

As per:
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#_configuration_files
The configuration files MUST be marked as such.
But also:
  "/usr is deemed to not contain configuration files in Fedora."

And I haven't found any exception in the Python-specific guidelines.

[NEED-2ND-OPINION] Can we get a guidance from a Python packaging expert?

Comment 6 Michal Schorm 2021-09-17 01:15:16 UTC
I've asked for help with this review from a Fedora Python SIG:
  https://lists.fedoraproject.org/archives/list/python-devel@lists.fedoraproject.org/thread/5HV356IFN4TGUKLC5O7N5RCP7CD6AYYC/

Comment 7 Miro Hrončok 2021-09-17 09:10:43 UTC
(In reply to Michal Schorm from comment #4)
> 1/ BuildRequire python3-devel [NEED-2ND-OPINION]
> [NEED-2ND-OPINION] "Every package that uses Python (at runtime and/or build
> time) and/or installs Python modules
>                    MUST explicitly include BuildRequires: python3-devel in
> its .spec file,
>                    even if Python is not actually invoked during build time."
> Reviewer note:
>   Your code:
>     BuildRequires:  python%{python3_pkgversion}-devel
>   I believe the Guideline explicitly shows the code that is mandatory to be
> included. And it is without macros.
>   _My understanding_ is that the pure following line MUST be used instead:
>     BuildRequires: python3-devel

The guideline is meant "effectively", hence python%{python3_pkgversion}-devel is actually fine.


> 2/ Naming [OK]
> Package name:
>   container-workflow-tool
> Canonical name:
>   | rpm --eval "%{py_dist_name container-workflow-tool}"
>   | container-workflow-tool
> Main importable module name:
>   container_workflow_tool
> 
> This package provides BOTH a tool (application) and the importable module.
> Currently the primarly function as I see it is to provide the application,
> so the following apply:
>   "Packages that primarily provide applications, services or any kind of
> executables
>   SHOULD be named according to the general Fedora naming guidelines (e.g.
> ansible)."

Ack.


> Also:
>   "Consider adding a virtual provide according to Library naming above
>   (e.g. python3-PROJECTNAME), if it would help users find the package."
> Since the goal is _also_ to provide the importable module, it might be a
> good idea to add such virtual provide.

The subpakcage with the importable module is already named python3-container-workflow-tool, so this is basically done. The main package should not also provide python3-container-workflow-tool, or there would be an ambiguity.

Note that I personally consider this split unnecessary, unless %{_bindir}/cwt is enormously big (which it isn't, it is merely a small entrypoint script).

Also note that if the split is kept, the cwt package needs to require fully-versioned python3-container-workflow-tool = %{?epoch:%{epoch}:}%{version}-%{release}.


> 3/ Files to include [OK]
> 
> [OK] "Packages MUST include the source file (*.py) AND
>      the bytecode cache (*.pyc) for each pure-Python importable module."
> [OK] "The source files MUST be included in the same package as the bytecode
> cache."
> [OK] "Scripts that are not importable (typically ones in %{_bindir} or
> %{_libexecdir})
>      SHOULD NOT be byte-compiled."
> [OK] "The cache files are found in a __pycache__ directory and
>      have an interpreter-dependent suffix like .cpython-39.pyc."

Ack.


> 4/ Dist-info metadata [OK]
> 
> [OK] "Each Python package MUST include Package Distribution Metadata
> conforming
>      to PyPA specifications (specifically, Recording installed
> distributons)."
> [OK] "The metadata SHOULD be included in the same subpackage as
>      the main importable module, if there is one."

Ack.


> 5/ Explicit lists [FAIL]
> [OK] "Packages MUST NOT own shared directories owned by Python itself"
> [FAIL] "Packagers SHOULD NOT simply glob everything under a shared
> directory."
> Reviewer note:
>   the package ship a single manual page. Please list the file explicitly,
> instead of:
>   |  %{_mandir}/man1/*

Ack. Although it is a SHOULD and not a MUST, so it is permitted to stay this way, but frowned upon.


> 6/ PyPI parity [FAIL]
> [FAIL] "Every Python package in Fedora SHOULD also be available on the
> Python Package Index (PyPI)."
> Reviewer note:
>   The attempt to make the project available via PyPI has yet not been made.
> [OK] "The command pip install PROJECTNAME MUST install the same package
>      (possibly in a different version), install nothing, or fail with a
> reasonable error message."
> [FAIL] "If your package is not or cannot be published on PyPI, you can:
>        * Ask Python SIG to block the name on PyPI for you"
> Reviewer note:
>   It is unclear whether the upstream would like to maintain the project in
> PyPI,
>   however to avoid future name collisions, it seems wise to me to block the
> name.
>   If the name collision would happen, you (maintainer) would be force to
> change the package name AND
>   you would have to try to convince upstream to rename its projects, since
> the names of the
>   upstream project and the Fedora package SHOULD match.
>   That is my understanding of the Python Naming Guidelines

Correct. If upstream does not plan to release on PyPI, you should block this name on PyPI. You can ping pviktori (encukou on #fedora-python IRC) to do that for you.
If upstream is willing to release on PyPI "eventually", please coordinate with them to upload a pre-release or similar to reserve the name.

I personally strongly suggest releasing on PyPI, since this is a Red Hat project, we have the power to do so.


> 7/ Provides and requirements [FAIL]
> |    Requires
> |    --------
> |    container-workflow-tool (rpmlib, GLIBC filtered):
> |        /usr/bin/python3
> |        python3-container-workflow-tool
> |    python3-container-workflow-tool (rpmlib, GLIBC filtered):
> |        python(abi)
> |        python3.10dist(gitpython)
> |        python3.10dist(pyyaml)
> |        python3.10dist(requests-kerberos)
> |    Provides
> |    --------
> |    container-workflow-tool:
> |        container-workflow-tool
> |    python3-container-workflow-tool:
> |        python-container-workflow-tool
> |        python3-container-workflow-tool
> |        python3.10-container-workflow-tool
> |        python3.10dist(container-workflow-tool)
> |        python3dist(container-workflow-tool)
> [OK] "For any module intended to be used in Python 3 with import MODNAME,
>      the package that includes it SHOULD provide python3-MODNAME,
>      with underscores (_) replaced by dashes (-)."
> [OK] "For any FOO, a package that provides python3-FOO SHOULD use
> %py_provides
>      or an automatic generator to also provide python-FOO and python3.X-FOO,
>      where X is the minor version of the interpreter."
> [OK] "The provide SHOULD NOT be added manually: if a generator or macro is
> not used,
>      do not add the python-FOO / python3.X-FOO provides at all."
> [OK] "Every Python package MUST provide python3dist(DISTNAME) and
> python3.Xdist(DISTNAME),
>      where X is the minor version of the interpreter and
>      DISTNAME is the Canonical project name corresponding to the Dist-info
> metadata."
> [OK] "This is generated automatically from the dist-info metadata.
>      The provide SHOULD NOT be added manually: if the generator fails to add
> it, the metadata MUST be fixed."
> [FAIL] "Each Python package MUST explicitly BuildRequire python3-devel."
> Reviewer note:
>   "BuildRequires:  python%{python3_pkgversion}-devel" is used instead.

That is fine, as said previously.

> [OK] "Packages MUST NOT have dependencies (either build-time or runtime)
>      with the unversioned prefix python- if the corresponding python3-
> dependency can be used instead."
> [OK] "Packages SHOULD NOT have explicit dependencies (either build-time or
> runtime)
>      with a minor-version prefix such as python3.8- or python3.8dist("
> [OK] "Packages SHOULD NOT have an explicit runtime dependency on python3."
> [NEED-2ND-OPINION] "Packages MUST use the automatic Python run-time
> dependency generator."
> [OK] "Packages SHOULD use the opt-in build-dependency generator if possible."
> [OK] "The packager MUST inspect the generated requires for correctness.
>      All dependencies MUST be resolvable within the targeted Fedora version."
> [OK] "Dependencies covered by the generators SHOULD NOT be repeated in the
> .spec file."
> [NEED-2ND-OPINION] The project README.md states that:
>   |  Requirements
>   |  ------------
>   |  * python3
>   |  * python3-GitPython
>   |  * python3-requests-kerberos
>   |  * fedpkg
>   |  * http://github.com/ficap/dhwebapi (installed using pip3 and
> requirements.txt)
>   But I see those requirements neither generated nor explicitly specified.
>   Shouldn't they be listed?

If the README is up to date, they should be listed, yes.

> 8/ Interpreter invocation [NEED-2ND-OPINION]
> [OK] "Shebang lines to invoke Python MUST use %{python3} as the interpreter."
> [OK] "Shebang lines to invoke Python SHOULD be #!%{python3}
> -%{py3_shebang_flags} and they MAY include extra flags."
> Reviewer note:
>   macro "%pyproject_install" is used.
>   | # grep -i -e "^#!" -r review-container-workflow-tool/rpms-unpacked/*
>   |
> review-container-workflow-tool/rpms-unpacked/container-workflow-tool-1.0.0-1.
> fc36.noarch.rpm/usr/bin/cwt:#! /usr/bin/python3 -s
> [NEED-2ND-OPINION] "Every executable TOOL for which the current version of
> Python matters
>                    SHOULD also be invokable by python3 -m TOOL."
>                    "If the software doesn’t provide this functionality,
> packagers SHOULD ask the upstream to add it."
> Reviewer note:
>   I don't know how to check if the current version of Python matters.

Does it matter for the user which Python version executes the tool? If the tool was written in Rust, it would need to be told (or detect/guess) what the Python version is to operate correctly?

If the answers are yes and yes, the current version of Python doesn't matter.
If the answers are no and no, the current version of Python doesn't matter.



> 9/ Using Cython [OK]
> [OK] "packages MUST NOT use files pre-generated by Cython"

Ack, does not use Cython at all.

> 10/ Tests [FAIL]
> [FAIL] "If a test suite exists upstream, it SHOULD be run in the %check
> section.
>        If that is not possible with reasonable effort,
>        at least a basic smoke test (such as importing the packaged module)
> MUST be run in %check."
> Reviewer note:
>   No test is currently run.
>   Even thought the justification why the upstream test are not run is OK, it
> does NOT follow the guidelines.
>   You MUST run atleast a basic smoke test.

Exactly.
Since this is a CLI tool *and* an importable module, I suggest something like:

%check
PYTHONPATH=%{buildroot}%{python3_sitelib} %{buildroot}%{_bindir}/cwt --help
%py3_check_import container_workflow_tool


> [OK] "In %check, packages SHOULD NOT run “linters”: code style checkers,
>      test coverage checkers and other tools that check code quality rather
> than functionality."

Ack.

> 11/ Source files from PyPI [OK]
> Reviewer note:
>   None used.

Ack.


Extra notes by me:

> %{?python_enable_dependency_generator}

This is deprecated and not needed.


> %global srcname container_workflow_tool

I consider defining this macro overengineering.


> # remove man pages from sitelib directory, where %%pyproject_install installed them
> rm -r %{buildroot}%{python3_sitelib}/usr

This should be fixed upstream.




> 12/ LICENSE file
> I do not see any usage of '%license' macro in the '%files' section, however
> the License file is present in the package:
>  python3-container-workflow-tool-1.0.0-1.fc36.noarch.rpm/usr/lib/python3.10/site-packages/container_workflow_tool-1.0.0.dist-info/LICENSE

Indeed. This is done by setuptools and on Fedora 35+ the file is detected by %pyproject_save_files and marked as %license. You can verify with: rpm -ql --licensefiles



> 13/ I can see the YAML config files in [/usr] ...

This depends on whether the user is supposed to edit those files.

If no, everything is fine.

If yes, they need to be in /etc and marked as configuration.

Comment 8 Petr Viktorin (pviktori) 2021-09-17 15:00:16 UTC
Anyone can upload to PyPI, so I did. The package is here: https://pypi.org/project/container-workflow-tool/1.0.0/#description

When you get an account on PyPI, let me know and I'll give you the rights to upload new versions. The steps to do this are:

$ sudo dnf install python3-pip python3-wheel /usr/bin/twine
$ python -m pip wheel -w wheels .
$ twine upload wheels/container_workflow_tool-*-py3-none-any.whl

Consider adding this to your release process, in order to be friendly to non-Fedora developers, testers who want older versions, etc.


Also consider not installing the man pages from setup.py -- that's a job for the specfile. Upstream Python packaging isn't good for system-wide installs.

Comment 9 Miro Hrončok 2021-09-17 15:10:43 UTC
> If the answers are yes and yes, the current version of Python doesn't matter.
> If the answers are no and no, the current version of Python doesn't matter.

There is a copy-paste error here. It should have said:

If the answers are yes and yes, the current version of Python does matter.
If the answers are no and no, the current version of Python doesn't matter.

Comment 10 Zuzana Miklankova 2021-09-20 05:39:14 UTC
Thank you all for a comprehensive feedback. I am going through a correction of the .spec file currently, and will commit the changes, as soon as they are ready.

Also, @pviktori , thank you for uploading the package to the PyPI database. I have just created an account (zmiklank) on the PyPI, so I could update the package by myself (as you said).

Comment 11 Zuzana Miklankova 2021-09-20 10:48:33 UTC
I have one additional question: What should be the name of the package if library package (python3-container-workflow-tool) and executable package (container-workflow-tool) are joined into one package, please?

In Fedora guidelines is written (as mschorm already mentioned):

Application naming

Packages that primarily provide applications, services or any kind of executables SHOULD be named according to the general Fedora naming guidelines (e.g. ansible). Consider adding a virtual provide according to Library naming above (e.g. python3-PROJECTNAME), if it would help users find the package.

But I would say that the main purpose of this package is to provide the importable module, not the runnable script.

Comment 12 Zuzana Miklankova 2021-09-21 15:10:37 UTC
The .spec file is changed, link to new COPR build is here: [1]
New .spec file can be found here [2], and srpm here [3].

For now I have joint the python3-container-workflow-tool and container-workflow-tool packages together, with name 'container-workflow-tool' (if answer to my above written question is different, I will change the name).
Regarding the .yaml conf files: they are not to be changed by the user, so I left them in /usr.

However, I discovered one obstacle while fixing the runtime requirements. It is the dhwebapi runtime dependency [4], for which the packages for Fedora nor PyPI exist. I started to write .spec file for the dhwebapi and started a communication with the upstream. Until dhwebapi's packages is published the cwt package is at stall.

[1] https://copr.fedorainfracloud.org/coprs/zmiklank/container-workflow-tool/build/2831925/
[2] https://copr-dist-git.fedorainfracloud.org/cgit/zmiklank/container-workflow-tool/container-workflow-tool.git/plain/container-workflow-tool.spec
[3] https://download.copr.fedorainfracloud.org/results/zmiklank/container-workflow-tool/fedora-34-x86_64/02831925-container-workflow-tool/container-workflow-tool-1.0.0-1.fc34.src.rpm
[4] https://github.com/ficap/dhwebapi/

Comment 13 Petr Viktorin (pviktori) 2021-09-29 07:55:29 UTC
I've added you as the PyPI package owner.

Comment 14 Zuzana Miklankova 2021-09-29 08:10:37 UTC
Thank you, @Petr Viktorin.



> However, I discovered one obstacle while fixing the runtime requirements. It is the dhwebapi runtime dependency [4], for which the packages for Fedora nor PyPI exist. I started to write .spec file for the dhwebapi and started a communication with the upstream. Until dhwebapi's packages is published the cwt package is at stall.

The dhwebapi dependency is removed in PR [1], thus the container-corkflow-tool is no longer blocked with dhwebapi package creation. I should be able to continue with work on the cwt after merging this PR.


[1] https://github.com/sclorg/container-workflow-tool/pull/45

Comment 15 Miro Hrončok 2021-09-29 09:28:34 UTC
Some more feedback on the current spec file:


> Provides:       python%{python3_pkgversion}-%{name}


Whenever you provide an "alternative package name" like this, always include the version:

  Provides:       python%{python3_pkgversion}-%{name} = %{?epoch:%{epoch}:}%{version}-%{release}

However, if the name starts with "python3-" you can just use "%py_provides python%{python3_pkgversion}-%{name}" which does this for you:

https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_provides_for_importable_modules


------------


# man installation change was proposed on upstream:
# https://github.com/sclorg/container-workflow-tool/pull/43
# if this PR is merged, the following lines can be removed

I don't understand that comment. The manual man page installation should stay when this is merged, no?



------

%files ...
%{_mandir}/man1/cwt.1.gz

You should not assume the suffix is gz. Based on some macros setup, it could change in the future. Usea glob like this instead:

%{_mandir}/man1/cwt.1*

Comment 16 Zuzana Miklankova 2021-09-29 12:35:44 UTC
Thanks for feedback.

> I don't understand that comment. The manual man page installation should stay when this is merged, no?
Sure, my mistake.

Today the container-workflow-tool 1.1.0 was released. This release comprises also removal of dhwebapi dependency and removal of man installation from setup.py. That means, that the container-workflow-tool .rpm package is correctly buildable from upstream code now.

Updated files are available as follows:
SPEC: https://copr-dist-git.fedorainfracloud.org/cgit/zmiklank/container-workflow-tool/container-workflow-tool.git/plain/container-workflow-tool.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/zmiklank/container-workflow-tool/fedora-34-x86_64/02863277-container-workflow-tool/container-workflow-tool-1.1.0-1.fc34.src.rpm
COPR build: https://copr.fedorainfracloud.org/coprs/zmiklank/container-workflow-tool/build/2863277/

Comment 17 Zuzana Miklankova 2021-10-11 11:06:06 UTC
cwt upstream had moved the 1.1.0 release tag a few commits further and thus currently the cwt 1.1.0 contains the pytest suite. The .spec needed to be rewritten a bit in order to follow this action:

SPEC: https://download.copr.fedorainfracloud.org/results/zmiklank/container-workflow-tool/fedora-34-x86_64/02883934-container-workflow-tool/container-workflow-tool.spec
SRPM: https://download.copr.fedorainfracloud.org/results/zmiklank/container-workflow-tool/fedora-34-x86_64/02883934-container-workflow-tool/container-workflow-tool-1.1.0-1.fc34.src.rpm
COPR build: https://copr.fedorainfracloud.org/coprs/zmiklank/container-workflow-tool/build/2883934/

cwt 1.1.0 was also already uploaded to the PyPI database.

Comment 19 Michal Schorm 2021-10-20 06:20:16 UTC
I've went throught the Python Packaging Guidelines again for this review.
I've found just two minor issues, neither which blocks this package to be accepted. 

This is a quick overview of what I went through:

Distro-wide guidelines          [OK]
Naming                          [OK]    
Files to include                [OK]
Dist-info metadata              [OK]
Explicit lists                  [OK]
PyPI parity                     [OK]
  https://pypi.org/project/container-workflow-tool/

Provides and requirements       [OK]
  The requirements from requirements.txt are listed:
    # rpm -q --requires container-workflow-tool-1.1.0-1.fc36.noarch.rpm
    ...
    python3.10dist(gitpython)
    python3.10dist(pyyaml)
    python3.10dist(requests-kerberos)
    ...

Interpreter invocation          [OK]
Using Cython                    [OK]
Tests                           [OK]
Source files from PyPI          [OK]

-----

Issue 1) Versioned links

During this round of the review, I wanted to compare it with the previous versions of the SPECfile.
However, nearly all of the links listed in this BZ now leads to the same version of the SPECfile.
I strongly suggest to keep an eye on this issue next time and let the links point to a specific commits, so the history of what has been done is not concealed.

Issue 2) Incomplete fix

Somewhere during your update from CWT version 1.0.0 to 1.1.0 you forgot to update the SPECfile changelog.
The listed date, when you claim you made an update to the 1.1.0-1 version: "Wed Sep 29 2021"
is before that version was released by upstream: "OCT-06-2021"
  https://github.com/sclorg/container-workflow-tool/releases/tag/1.1.0

There was IMO also no need to re-write the SPECfile, instead of patching it.
You could have kept the changelog and list everything you fixed in it.
This would also partly help with comparing between the older versions of the SPECfile.

-----

I _ACCEPT_ this package submission.
Zuzana, feel free to contact me anytime, should you need any additional guidance or just a help in the Fedora import/build/update process.

Thank you Miro for the help.

Comment 20 Zuzana Miklankova 2021-10-20 08:55:50 UTC
Thank you for reviewing the package.

>During this round of the review, I wanted to compare it with the previous versions of the SPECfile.
>However, nearly all of the links listed in this BZ now leads to the same version of the SPECfile.
>I strongly suggest to keep an eye on this issue next time and let the links point to a specific commits, so the history of what has been done is not concealed.

I noticed this also, but it was too late at that point. I will definitely try to avoid this mistake in the future.

Comment 21 Miroslav Suchý 2021-10-25 07:54:25 UTC
I have sponsored Zuzana to the packager group.

Comment 22 Gwyn Ciesla 2021-10-26 13:30:57 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/container-workflow-tool


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