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 2-Clause License". 16 files have
     unknown license. Detailed output of licensecheck in
     /home/fedora/2244402-python-ebcdic/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
[-]: 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.
[x]: 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.
[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: There are rpmlint messages (see attachment).
[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]: The License field must be a valid SPDX expression.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 8754 bytes in 2 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[-]: Binary eggs must be removed in %prep
     Note: Cannot find any build in BUILD directory (--prebuilt option?)
[-]: 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

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

Generic:
[-]: 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).
[ ]: 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-ebcdic-1.1.1-2.fc40.noarch.rpm
          python-ebcdic-1.1.1-2.fc40.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/tmp5v3829pj')]
checks: 31, packages: 2

python-ebcdic.spec: W: patch-not-applied Patch0: %{forgeurl}/pull/17.patch
 2 packages and 0 specfiles checked; 0 errors, 1 warnings, 0 badness; has taken 0.2 s 




Rpmlint (installed packages)
----------------------------
============================ rpmlint session starts ============================
rpmlint: 2.4.0
configuration:
    /usr/lib/python3.12/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.1 s 



Source checksums
----------------
https://github.com/roskakori/CodecMapper/archive/v1.1.1/CodecMapper-1.1.1.tar.gz :
  CHECKSUM(SHA256) this package     : 7a1a77fdc7e87924e42826087bd9c0c4b48b779156c10cabc94eec237739c818
  CHECKSUM(SHA256) upstream package : 7a1a77fdc7e87924e42826087bd9c0c4b48b779156c10cabc94eec237739c818


Requires
--------
python3-ebcdic (rpmlib, GLIBC filtered):
    python(abi)



Provides
--------
python3-ebcdic:
    python-ebcdic
    python3-ebcdic
    python3.12-ebcdic
    python3.12dist(ebcdic)
    python3dist(ebcdic)



Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24
Command line :/usr/bin/fedora-review -b 2244402
Buildroot used: fedora-rawhide-aarch64
Active plugins: Shell-api, Generic, Python
Disabled plugins: Perl, SugarActivity, PHP, fonts, C/C++, Haskell, R, Java, Ocaml
Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH

Comments:
a) Ant generates warnings such as:
/usr/lib/python3.12/site-packages/setuptools/command/test.py:195: _DeprecatedInstaller: setuptools.installer an
d fetch_build_eggs are deprecated.
May want to update use to just generate the required files, possibly by modifying:
https://github.com/roskakori/CodecMapper/blob/master/build.xml
In particular, the Python tests should not be called in the prep section.
PRep section should just move, modify or delete files.  Generation of new files should
probably be in the build section. This may require further folder re-organization.

Thank you for the review.

Regarding your comment:

Technically, calling `ant test` is a preparation for the build that follows. So, I feel that %prep is the right step in the build process to do so. On the other hand, I haven't looked into or tried to understand what the script does under the hood. I simply followed the instructions given. I know it extracts codecs from Java and prepares them for use in Python.

However, it appears that I can be more specific and call `ant ebcdic`, which will only generate the codecs. It gets rid of the warnings and I can drop the BR on python3-pycodestyle.

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/extract-msg/fedora-rawhide-x86_64/06532732-python-ebcdic/python-ebcdic.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/extract-msg/fedora-rawhide-x86_64/06532732-python-ebcdic/python-ebcdic-1.1.1-3.fc40.src.rpm

Ping?

The prep section should not do more than use commands such as mv, rm, sed etc.

ant ebcdic compiles the java file at:
https://github.com/roskakori/CodecMapper/tree/master/source/at/roskakori/codecmapper
and then generates the codec files:
https://github.com/roskakori/CodecMapper/blob/master/build.xml#L43

It should therefore be in the build section.

For the url, it is sufficient to have:
URL:            https://github.com/roskakori/CodecMapper

(In reply to Benson Muite from comment #4)
> The prep section should not do more than use commands such as mv, rm, sed
> etc.
> 
> ant ebcdic compiles the java file at:
> https://github.com/roskakori/CodecMapper/tree/master/source/at/roskakori/
> codecmapper
> and then generates the codec files:
> https://github.com/roskakori/CodecMapper/blob/master/build.xml#L43
> 
> It should therefore be in the build section.

Since SHOULD does not mean a mandatory requirement, I feel I'm still within my liberties to diverge. Could you point me to a relevant section of the packaging guidelines, so I can read up on it?

> 
> For the url, it is sufficient to have:
> URL:            https://github.com/roskakori/CodecMapper

I chose the somewhat longer URL since that has the `README.rst` displayed and gives information relevant to `EBCDIC`. But I'm fine with using `%forgeurl` instead, since the package does need code from `CodecMapper` to be useful.

Information about what should be done in each section could be
clarified better in the build guidelines. For reference, see:
https://rpm-software-management.github.io/rpm/manual/spec.html
https://docs.fedoraproject.org/en-US/java-packaging-howto/introduction_for_developers/

the %prep section is to unpack sources and apply patches. Running existing programs
that modify the sources such as iconv is often done, but other commands
associated with setting up the build environment such as using cmake, autotools etc.
are done in build sections.

Compiling code is best done in the %build section as a cleanable
directory hierarchy is created for that purpose. Need to go through:
https://github.com/rpm-software-management/rpm/blob/master/tools/rpmbuild.c
in more detail.

(In reply to Benson Muite from comment #6)
> Information about what should be done in each section could be
> clarified better in the build guidelines. For reference, see:
> https://rpm-software-management.github.io/rpm/manual/spec.html

"""
%prep prepares the sources for building. This is where sources are unpacked and possible patches applied, and other similar activies could be performed.
"""

Isn't that exactly what I'm doing? That the sources are prepared using a Java tool shouldn't matter. It's still a preparation for the build to come. Since I need to run `%pyproject_buildrequires` before `%build`, I really don't see how I could move running the ant script to build. By the time I get there the script needs to be gone. We are building a Python package not a Java package. And the Python package lives in `ebcdic/`, which we move to the top level, so the Python RPM macros can do their magic.

> https://docs.fedoraproject.org/en-US/java-packaging-howto/
> introduction_for_developers/

This isn't a Java package. It's merely using a Java tool for prepping the sources. I could not run ant at all. But that would result in an incomplete set of codecs. Yet the package would still build.

> the %prep section is to unpack sources and apply patches. Running existing
> programs
> that modify the sources such as iconv is often done, but other commands
> associated with setting up the build environment such as using cmake,
> autotools etc.
> are done in build sections.
> 
> Compiling code is best done in the %build section as a cleanable
> directory hierarchy is created for that purpose. Need to go through:
> https://github.com/rpm-software-management/rpm/blob/master/tools/rpmbuild.c
> in more detail.

That's going into the nitty-gritty of RPM itself. I was looking more for a section in the Fedora Packaging Guidelines saying that what I do MUST not be done.

Probably it is easier for someone else to complete the review.
Sorry for delaying it.

Thanks for your time and for the review. I will discuss with the packaging folks regarding the unresolved issue.

Please also update the packaging guidelines appropriately. It can be done by a pull request!

One can put the ant in the build section.
Use of %pyproject_buildrequires is a should if possible
https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#Automatically-generated-dependencies
But there may be better options
https://koji.fedoraproject.org/koji/taskinfo?taskID=108352799

(In reply to Benson Muite from comment #11)
> One can put the ant in the build section.

I never said one couldn't. I'd rather keep `%pyproject_buildrequires`, so I don't have to track dependencies manually, which also may change over time.

> Use of %pyproject_buildrequires is a should if possible
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/
> #Automatically-generated-dependencies

Indeed it's not a MUST, but very helpful. But what we are really after is a statement that says: Packages MUST NOT run scripts in %prep. Because that would demand me finding a better solution. So, what we have is two SHOULDs and we are weighing these differently.

> But there may be better options
> https://koji.fedoraproject.org/koji/taskinfo?taskID=108352799

Thanks for the effort! I get your point. I'm just still not convinced regarding the trade off. I'd rather you gave your opinion on the mailing list[1], then here in the bug.

[1] https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/R7TC3LYD57T3F35BSHXEJSKSGID3MXZA/

I agree with Benson. Running ant test that generates code should not be run in %prep.

(In reply to Maxwell G from comment #13)
> I agree with Benson. Running ant test that generates code should not be run
> in %prep.

Is there anything I can do to convince you that the trade off is more painful wrt to package maintenance? If I could _easily_ avoid it, I would.

(In reply to Sandro from comment #14)
> (In reply to Maxwell G from comment #13)
> > I agree with Benson. Running ant test that generates code should not be run
> > in %prep.
> 
> Is there anything I can do to convince you that the trade off is more
> painful wrt to package maintenance? If I could _easily_ avoid it, I would.

How about:

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06612269-python-ebcdic/python-ebcdic.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06612269-python-ebcdic/python-ebcdic-1.1.1-4.fc40.src.rpm

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6612296
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2244402-python-ebcdic/fedora-rawhide-x86_64/06612296-python-ebcdic/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Maxwell's Python Package Review Template
==============

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


- [x] The License tag reflects the package contents and uses the correct identifiers.
- [x] The license text is included and marked with %license.
- [x] The package builds successfully in mock.
- [x] The package is installable (checked by fedora-review).
- [x] There are no relevant rpmlint errors.


- [x] The package runs tests in %check.

Yes, but the `%pytest -v ebcdic/test/test_ebcdic.py` syntax is generally preferred.

- [x] The latest version is packaged or packaging an earlier version is justified.


- [!] The packager considers avoiding confusing `%foo_name` macros. (Not a blocker)

It's generally preferred to use the literal value everywhere instead of a %{pypi_name} macro

- [x] Libraries: The package name has a `python3-` prefix and uses the canonical project name
- [x] The pyproject macros are used.
- [x] There are no bundled libraries.
- [x] The package complies with the Python and general Packaging Guidelines.

- [!] Package does not run builds outside of %build

`ant ebcdic` is not a configure script. The `ebcdic` target's description says "_build_ ebcdic package for Python." It calls `setup.py` and other build tasks. Please move it to %build.

(In reply to Maxwell G from comment #17)
> - [!] Package does not run builds outside of %build
> 
> `ant ebcdic` is not a configure script. The `ebcdic` target's description
> says "_build_ ebcdic package for Python." It calls `setup.py` and other
> build tasks. Please move it to %build.

It is in the context of `ebcdic` package.

I removed the call to `setup.py` from `build.xml`. I cannot move `ant ebcdic` to %build without extensive modification to the entire environment. I'd also like to keep using `%pyproject_buildrequires` for dynamic requirements generation. I think I've already come a long way from the initial proposal of running `ant ebcdic` in %prep. Seeing that everything ant produces besides the codecs itself, is subsequently removed. I don't see any harm of running the script in either %prep or %conf.

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06624382-python-ebcdic/python-ebcdic.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06624382-python-ebcdic/python-ebcdic-1.1.1-5.fc40.src.rpm

Created attachment 1998623 [details]
The .spec file difference from Copr build 6612296 to 6624391

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6624391
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2244402-python-ebcdic/fedora-rawhide-x86_64/06624391-python-ebcdic/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

> I cannot move `ant ebcdic` to %build without extensive modification to the entire environment.

I don't follow.

(In reply to Maxwell G from comment #21)
> > I cannot move `ant ebcdic` to %build without extensive modification to the entire environment.
> 
> I don't follow.

Could you elaborate, please? I'm assuming you read through the previous comments.

I don't understand what "extensive modification" would need to be made.

Well, doing everything in %build as Benson suggested, I would need to drop %pyproject_buildrequires and track dependencies myself. While possible, I don't like that idea.

So far, nobody has pointed out a 'MUST NOT' item in the packaging guidelines (here or on the mailing list), that running `ant ebcdic` in %prep would violate. It is my opinion, while running ant in general is a build step, in this case it is simply a preparatory step for the build that follows. An elaborate way of some sort of applying patches.  It extracts additional codecs for ebcdic. Whatever else is created on the fly, is not included in the package. Only the contents in directory `ebcdic/` matter for the Python package. So, I'm moving that into place and remove anything else preparing for the build.

I have come forward and moved the preparation into %conf as was suggested on the mailing list. Now, moving everything into build, this would get rather messy, imho. It doesn't solve much either. The package would still BR ant and whatever side effects running ant has would still be removed before the actual build starts. I also just realized, while %pyproject-buildrequires is still in the spec file, it has become obsolete. It seems it needs to be run in %generate_buildrequires. So, using %conf proves to be no option either, if I want to keep %pyproject_buildrequires. And I do.

Unless there is a rule in the packaging guidelines which forbids running ant in %prep, I don't feel like this should be blocking the review at all.

I believe I have addressed all other issues pointed out. I patched build.xml, so it no longer calls ebcdic/setup.py. I'm running `ant ebcdic` to make sure no linters are required, etc. etc.

Hypothetically, imagine some upstream developer very fond of `make`, using it for everything. That developer uses make for benign tasks. Would I not be allowed to run `make foo` in %prep either? Even if all that `make foo` does, is moving around a handful of files?

> Well, doing everything in %build as Benson suggested, I would need to drop %pyproject_buildrequires and track dependencies myself. While possible, I don't like that idea.

How about https://paste.sr.ht/~gotmax23/0c1a84951b856294dd36160d0c38f2bc31365882?

Spec URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06629772-python-ebcdic/python-ebcdic.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/gui1ty/reviews/fedora-rawhide-x86_64/06629772-python-ebcdic/python-ebcdic-1.1.1-6.fc40.src.rpm

Created attachment 1999159 [details]
The .spec file difference from Copr build 6624391 to 6630301

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6630301
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2244402-python-ebcdic/fedora-rawhide-x86_64/06630301-python-ebcdic/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

I'm closing this review requests in favor of maintaining extract-msg and its dependencies in Copr [1]. In the end extract-msg is just a niche package. I would have liked to make it available in Fedora proper, but my time is limited. What's in Copr now is usable. So, I decided to leave that in place and stop pursuing an import into Fedora repositories.

Thanks for your time!

[1] https://copr.fedorainfracloud.org/coprs/gui1ty/extract-msg/