A: you don't gzip man files, rpmbuild does it.

gzip %{buildroot}/%{_mandir}/man1/*.1
gzip %{buildroot}/%{_mandir}/man5/*.5


B: You don't include the .gz extension in files.


%doc %{_mandir}/man1/*.1.gz
%doc %{_mandir}/man5/*.5.gz


C: man pages aren't considered docs or have the packaging guidelines changed recently?

%doc %{_mandir}/man1/*.1.gz
%doc %{_mandir}/man5/*.5.gz

(In reply to leigh scott from comment #1)
> A: you don't gzip man files, rpmbuild does it.
> 
> gzip %{buildroot}/%{_mandir}/man1/*.1
> gzip %{buildroot}/%{_mandir}/man5/*.5
> 
> 
> B: You don't include the .gz extension in files.
> 
> 
> %doc %{_mandir}/man1/*.1.gz
> %doc %{_mandir}/man5/*.5.gz
> 
> 
> C: man pages aren't considered docs or have the packaging guidelines changed
> recently?
> 
> %doc %{_mandir}/man1/*.1.gz
> %doc %{_mandir}/man5/*.5.gz

see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

Minor: according to https://github.com/nih-at/nihtest/blob/main/README.md
nihtest-case.1 should be nihtest-case.5

(In reply to leigh scott from comment #1)
> A: you don't gzip man files, rpmbuild does it.
> 
> gzip %{buildroot}/%{_mandir}/man1/*.1
> gzip %{buildroot}/%{_mandir}/man5/*.5

Oh, I didn't know. I haven't had to deal with man pages often.
 
> B: You don't include the .gz extension in files.
> 
> 
> %doc %{_mandir}/man1/*.1.gz
> %doc %{_mandir}/man5/*.5.gz

Right. Although it doesn't hurt either (see C).

> C: man pages aren't considered docs or have the packaging guidelines changed
> recently?
> 
> %doc %{_mandir}/man1/*.1.gz
> %doc %{_mandir}/man5/*.5.gz

They are. The link you posted in comment #2 states:

"Note also that files installed in %{_mandir} are automatically marked by RPM as documentation. Thus it is not necessary to use %doc."

You can also see that by, e.g., looking at git with 'rpm -qd git'. It will list installed manpages. So, while '%doc' is not needed, it doesn't hurt either.

I've done A and B in another package before and rpmlint didn't complain. Not sure if that was before or after package review. Anyway, I'll change it.

(In reply to Remi Collet from comment #3)
> Minor: according to https://github.com/nih-at/nihtest/blob/main/README.md
> nihtest-case.1 should be nihtest-case.5

You are correct. Oversight on my part. 'mandoc -l nihtest-case.man' also shows it should be nihtest-case.5. I will correct that.

Updated sources:

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06112141-python-nihtest/python-nihtest.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06112141-python-nihtest/python-nihtest-1.1.0-7.fc39.src.rpm

Above build also has the full test suite enabled.

The tests do not run. May want to do something like:

cmake -S .
make
make test

or using the macros
%cmake
%cmake_build
%ctest

The CMakeLists file creates a virtual env and then uses that
to run the test:
https://github.com/nih-at/nihtest/blob/main/CMakeLists.txt#L29-L30

This does not work in the build system. Is there a good way to 
use nihtest in %{buildroot}%{_bindir}/nihtest ?

Side note (please ignore it, probably no value)

I will probably never understand python Guidelines
I may understand the python3-xxx for a library, as this allows also to have some python3.10-xxx and python3.12-xxx (parallel installable)
But this project is "nihtest", and provides the "nihtest" command and man pages which will make parallel installation impossible
IMHO, for an application, the python namespace doesn't make any sense.

Well, Guidelines are a  bit confusing

> The rest of the Guidelines apply to packages that ship code that can be imported with Python’s import statement.
> Specifically, that is all packages that install files under /usr/lib*/python*/.
>
> Except for the two “Distro-wide guidelines”, these Guidelines do not apply to simple one-file scripts or utilities,
> especially if these are included with software not written in Python. However, if an application (e.g. CLI tool,
> script or GUI app) needs a more complex Python library, the library SHOULD be packaged as an importable library
> under these guidelines.

(In reply to Benson Muite from comment #6)
> The tests do not run. May want to do something like:
> 
> cmake -S .
> make
> make test
> 
> or using the macros
> %cmake
> %cmake_build
> %ctest
> 
> The CMakeLists file creates a virtual env and then uses that
> to run the test:
> https://github.com/nih-at/nihtest/blob/main/CMakeLists.txt#L29-L30
> 
> This does not work in the build system. Is there a good way to 
> use nihtest in %{buildroot}%{_bindir}/nihtest ?

It took me a while to understand what you meant and to understand how the tests actually work. I'm not a CMake expert. Not using it often.

Anyway, I tried locally using the macros and it indeed fails trying to install dependencies using pip. What I fail to deduct is what dependencies it is trying to install. If I can determine that, I could probably add these a BRs and modify the CMakeLists.txt, so it doesn't run pip.

Otherwise, reverting to only running %pyproject_check_import may be the only option.



(In reply to Remi Collet from comment #7)
> Side note (please ignore it, probably no value)

That's hard... ;)
 
> I will probably never understand python Guidelines
> I may understand the python3-xxx for a library, as this allows also to have
> some python3.10-xxx and python3.12-xxx (parallel installable)
> But this project is "nihtest", and provides the "nihtest" command and man
> pages which will make parallel installation impossible
> IMHO, for an application, the python namespace doesn't make any sense.

Well, I had similar discussions regarding package names before. It boils down to what the main purpose of the package is. Is it primarily an application or commandline tool or is it primarily a library to be used by other packages.
If in doubt or if it really is both, there's an option to add a Provides for the binary, so it's easier to find. I did that for another Python package: https://src.fedoraproject.org/rpms/python-fvs/blob/rawhide/f/python-fvs.spec#_37
Since we are still in review, I'd be happy to add that to the spec, so that 'dnf install nihtest' will work by pulling in python3-nihtest.

If you install nihtest yourself locally by using
pip install --user
from the source directory, the tests will complete correctly.  Using a local folder
pip install --t my_nihtest_install
and then adding this to PYTHONPATH does not work though.

Side note: libzip 1.10.0 build and test suite passes using this package (even if not yet really ready)

(In reply to Remi Collet from comment #11)
> Side note: libzip 1.10.0 build and test suite passes using this package
> (even if not yet really ready)

Excellent!

I'll take another look at the tests, time permitting. If it turns out to be too cumbersome, I'll resubmit without running tests, relying on %pyproject_check_import only. Sorry about the delay.

Probably one can determine commands to update in the CMakeLists files from the
Python macro files
https://src.fedoraproject.org/rpms/python-rpm-macros
https://src.fedoraproject.org/rpms/pyproject-rpm-macros

These can be removed:
https://github.com/nih-at/nihtest/blob/main/CMakeLists.txt#L14-L21
https://github.com/nih-at/nihtest/blob/main/CMakeLists.txt#L29-L30

This
https://github.com/nih-at/nihtest/blob/main/CMakeLists.txt#L23
can be changed to point to the nihtest installation done by Fedora packaging

MAy need to update:
https://github.com/nih-at/nihtest/blob/main/tests/CMakeLists.txt#L46
to set PYTHONPATH, PATH and other environment variables that are
setup when calling %pyproject_check_import
https://src.fedoraproject.org/rpms/pyproject-rpm-macros/blob/rawhide/f/macros.pyproject#_134
https://src.fedoraproject.org/rpms/python-rpm-macros/blob/rawhide/f/macros.python3#_75

Thank you very much for the help and the links.

It's pretty much what I was thinking of. Patch the CMakeLists.txt file(s) to make it work in the build environment without rebuilding the already build binaries / modules.

I feel I'm almost there. However, I've hit a roadblock:

Traceback (most recent call last):
  File "/builddir/build/BUILDROOT/python-nihtest-1.1.0-8.fc39.x86_64/usr/bin/nihtest", line 5, in <module>
    from nihtest.__main__ import main
ModuleNotFoundError: No module named 'nihtest'

I defined the environment in the spec file before calling ctest (copied from the %pyproject_check_import macro):

# Run tests using CMake
%if %{with tests}
  %cmake
  %cmake_build
  # Set up environment
  PATH="%{buildroot}%{_bindir}:$PATH"
  PYTHONPATH="${PYTHONPATH:-%{buildroot}%{python3_sitearch}:%{buildroot}%{python3_sitelib}}"
  _PYTHONSITE="%{buildroot}%{python3_sitearch}:%{buildroot}%{python3_sitelib}"
  %ctest
%endif

Maybe I need to do that in the CMakeLists.txt. I'll dig some more tomorrow. Too tired to continue now.

In order not to delay things further, I disabled the CMake test, relying on %pyproject_check_import for now. I will continue with it, but right now I cannot find the time.

Updated sources:

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06123832-python-nihtest/python-nihtest.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06123832-python-nihtest/python-nihtest-1.1.0-8.fc39.src.rpm

(PS: I forgot to hit 'Save Changes' when writing this up a few days ago. :-\ Sorry for causing an unnecessary delay after all...)

Raised an issue:
https://github.com/nih-at/nihtest/issues/5
Maybe wait a day or two.  There has also been release 1.1.1

(In reply to Benson Muite from comment #16)
> Raised an issue:
> https://github.com/nih-at/nihtest/issues/5

Thanks. I'll keep an eye on it.

> Maybe wait a day or two.  There has also been release 1.1.1

Ah! Hadn't noticed yet. I'll provide updated sources some time this week.

Adding the following to the spec file (probably better done with patches) seems to enable the tests to run:

%check
%pyproject_check_import
touch check.sh
echo "PATH=%{buildroot}%{_bindir}:${PATH} PYTHONPATH=%{buildroot}%{python3_sitearch}:%{buildroot}%{python3_sitelib}:${PYTHONPATH} %{python3} %{buildroot}%{_bindir}/nihtest -v \$1 " >> check.sh
# Update CMakelists configuration to use installed nihtest
sed -i 's|${NIHTEST}|bash %{_builddir}/%{pypi_name}-%{version}/check.sh|g' tests/CMakeLists.txt
sed -i 's|ENVIRONMENT "PATH=${path}"|ENVIRONMENT "PYTHONPATH=%{buildroot}%{python3_sitearch}:%{buildroot}%{python3_sitelib}:$ENV{PYTHONPATH}"|g' tests/CMakeLists.txt
sed -i 's|set|#set|g' CMakeLists.txt
sed -i 's|add_custom|#add_custom|g' CMakeLists.txt
sed -i 's|COMMAND|#COMMAND|g' CMakeLists.txt
sed -i 's|OUTPUT|#OUTPUT|g' CMakeLists.txt
sed -i 's|DEPENDS|#DEPENDS|g' CMakeLists.txt
sed -i 's|^)|#)|g' CMakeLists.txt 
echo "" >> tests/nihtest.conf.in
echo "[environment]" >> tests/nihtest.conf.in
echo "PYTHONPATH=%{buildroot}%{python3_sitearch}:%{buildroot}%{python3_sitelib}" >> tests/nihtest.conf.in
%cmake
%cmake_build
%ctest

Thanks a lot for sorting this uit! Much appreciated. I put it into the spec file as part patch, part inline modifications.

Updated sources:

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06154896-python-nihtest/python-nihtest.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06154896-python-nihtest/python-nihtest-1.1.1-1.fc39.src.rpm

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

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


Issues:
=======
- Dist tag is present.


===== 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "BSD 3-Clause License". 118 files have
     unknown license. Detailed output of licensecheck in
     /home/FedoraPackaging/my-packages/python-
     nihtest/2217257-python-nihtest/licensecheck.txt
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (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]: Requires correct, justified where necessary.
[-]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 4 files.
[x]: Package complies to the Packaging Guidelines
[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: No rpmlint messages.
[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 %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[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

Python:
[-]: 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]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: Binary eggs must be removed in %prep

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

Generic:
[!]: Uses parallel make %{?_smp_mflags} macro.
[-]: 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.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[ ]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define autorelease(e:s:pb:n)
     %{?-p:0.}%{lua:
[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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: python3-nihtest-1.1.1-1.fc38.noarch.rpm
          python-nihtest-1.1.1-1.fc38.src.rpm
==================================================== rpmlint session starts ====================================================
rpmlint: 2.4.0
configuration:
    /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
rpmlintrc: [PosixPath('/tmp/tmpc8rmkh2e')]
checks: 31, packages: 2

===================== 2 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 7.2 s =====================




Rpmlint (installed packages)
----------------------------
============================ rpmlint session starts ============================
rpmlint: 2.4.0
configuration:
    /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
checks: 31, packages: 1

 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.6 s 



Source checksums
----------------
https://github.com/nih-at/nihtest/archive/v1.1.1/nihtest-1.1.1.tar.gz :
  CHECKSUM(SHA256) this package     : 81f6f180bfa4d450dc8a53da3a1949804e41ef4046843760ccfdc4c2eca6fb57
  CHECKSUM(SHA256) upstream package : 81f6f180bfa4d450dc8a53da3a1949804e41ef4046843760ccfdc4c2eca6fb57


Requires
--------
python3-nihtest (rpmlib, GLIBC filtered):
    /usr/bin/python3
    python(abi)



Provides
--------
python3-nihtest:
    nihtest
    python-nihtest
    python3-nihtest
    python3.11-nihtest
    python3.11dist(nihtest)
    python3dist(nihtest)



Generated by fedora-review 0.9.0 (6761b6c) last change: 2022-08-23
Command line :/usr/bin/fedora-review -b 2217257 -m fedora-38-x86_64
Buildroot used: fedora-38-x86_64
Active plugins: Generic, Shell-api, Python
Disabled plugins: Ocaml, PHP, SugarActivity, Haskell, C/C++, fonts, Ruby, Java, R, Perl
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comments:
a) Seems ok, thanks for the updates.
b) Should naming be modified slightly because it is an application:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_application_naming
Can likely just use nihtest, see for example:
https://src.fedoraproject.org/rpms/ansible/blob/rawhide/f/ansible.spec
c) Can parallel make be used for the documentation generation (not essential).

Regarding your questions / requests:

b) I've been down that road before. There's multiple things to consider:

I) PyPI parity: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_pypi_parity
II) Primary use of the package (library vs. utility / app): https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_library_naming vs. https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_application_naming
III) Provides and requirements: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_provides_and_requirements

I) is not relevant. The package is registered on PyPI as 'nihtest'. Regarding II) the package provides an application but also a library. In this case I find it hard to say which is prevalent. I'd say both depend on each other. The app requires the library, but one may also import the library to extend it etc.
That's why I opted for providing 'nihtest' as a virtual sub package. From a packaging approach, I find it easier than the other way around. By providing 'nihtest' a user can simply install with 'dnf install nihtest' if in need of the app.

c) I added the '%{?_smp_mflags}' macro to the make command.

Updated sources:

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06155317-python-nihtest/python-nihtest.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06155317-python-nihtest/python-nihtest-1.1.1-2.fc39.src.rpm

Created attachment 1974903 [details]
The .spec file difference from Copr build 6154922 to 6155350

Thanks. Approved.

The Pagure repository was created at https://src.fedoraproject.org/rpms/python-nihtest

FEDORA-2023-25dcfc16aa has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-25dcfc16aa

FEDORA-2023-25dcfc16aa has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Thanks all for the work on this package.

Using it, was able to build
libzip-1.10.0 https://bodhi.fedoraproject.org/updates/FEDORA-2023-78edd40e35
php-pecl-libzip-1.22.1 https://bodhi.fedoraproject.org/updates/FEDORA-2023-90759d845d