Bug 2460052

spec: https://download.copr.fedorainfracloud.org/results/averbyts/I3313/fedora-rawhide-x86_64/10352098-python-iso4217/python-iso4217.spec
srpm: https://packages.redhat.com/api/pulp-content/public-copr/averbyts/I3313/fedora-44-x86_64/Packages/p/python-iso4217-1.16-1.fc44.src.rpm

Cannot find any valid SRPM URL for this ticket. Common causes are:

- You didn't specify `SRPM URL: ...` in the ticket description
  or any of your comments
- The URL schema isn't HTTP or HTTPS
- The SRPM package linked in your URL doesn't match the package name specified
  in the ticket summary


---
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.

Copr build:
https://copr.fedorainfracloud.org/coprs/build/10355232
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2460052-python-iso4217/fedora-rawhide-x86_64/10355232-python-iso4217/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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.

New packages “MUST” use the current Python packaging guidelines with pyproject-rpm-macros, https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/, rather than the “201x-era” Python packaging guidelines, https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/, as in this submission.

This is a simple package, so you should find it straightforward to update it to current practices. See https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_example_spec_file for an example.

This is particularly messy and weird:

  Provides: python%{python3_version}dist(iso4217)

This is pointless because Provides will be generated automatically, https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_provides_for_importable_modules, and this style, using a modern python3dist(…) dependency with the %{python3_version} is a weird mishmash that I wouldn’t expect someone to come up with manually. Even an outdated tool like pyp2rpm shouldn’t emit something this weird. Are you using an LLM to author these spec files?

“Public Domain” is not a valid SPDX license expression. It should be (assuming you have identified the license correctly, which I haven’t checked) LicenseRef-Fedora-Public-Domain, and you must submit the license text for validation, similar to https://gitlab.com/fedora/legal/fedora-license-data/-/work_items/728. See https://docs.fedoraproject.org/en-US/legal/update-existing-packages/#_licenseref_callaway_public_domain, which mostly applies to new packages even though it is under the documentation for updating existing packages to SPDX.

Dear Ben,

Many thanks for the suggestions. I've implemented them, however the license issue is not very clear to me.
I've submitted a PR in the upstream with the explicit LICENSE file and it was accepted https://github.com/dahlia/iso4217/pull/30, so I would assume one can just create a LICENSE file in the spec manually.


spec: https://download.copr.fedorainfracloud.org/results/averbyts/I3313/fedora-rawhide-x86_64/10356720-python-iso4217/python-iso4217.spec
srpm: https://packages.redhat.com/api/pulp-content/public-copr/averbyts/I3313/fedora-rawhide-x86_64/Packages/p/python-iso4217-1.16-1.fc45.src.rpm

Created attachment 2138037 [details]
The .spec file difference from Copr build 10355232 to 10359799

Copr build:
https://copr.fedorainfracloud.org/coprs/build/10359799
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2460052-python-iso4217/fedora-rawhide-x86_64/10359799-python-iso4217/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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.

LICENSE file now matches upstream

spec: https://download.copr.fedorainfracloud.org/results/averbyts/I3313/fedora-44-x86_64/10359847-python-iso4217/python-iso4217.spec
srpm: https://packages.redhat.com/api/pulp-content/public-copr/averbyts/I3313/fedora-rawhide-x86_64/Packages/p/python-iso4217-1.16-1.fc45.src.rpm

Created attachment 2138039 [details]
The .spec file difference from Copr build 10359799 to 10359855

Copr build:
https://copr.fedorainfracloud.org/coprs/build/10359855
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2460052-python-iso4217/fedora-rawhide-x86_64/10359855-python-iso4217/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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.

(In reply to andrii.verbytskyi from comment #5)
> Dear Ben,
> 
> Many thanks for the suggestions. I've implemented them, however the license
> issue is not very clear to me.
> I've submitted a PR in the upstream with the explicit LICENSE file and it
> was accepted https://github.com/dahlia/iso4217/pull/30, so I would assume
> one can just create a LICENSE file in the spec manually.

Thanks, this is starting to look better.

----

The LICENSE file is nice, but it’s not mandatory in this case.

Please have a look at https://gitlab.com/fedora/legal/fedora-license-data/-/work_items/728 for an example. Since public-domain declarations are not standardized and take many forms (and since in theory something that looks like a public-domain declaration might have other conditions attached that make it ineffective, although this is unusual), Fedora Legal asks that all public-domain dedications be submitted for review and recorded in the file public-domain-text.txt in the fedora-license-data package.

1. Create an issue (“work item”) on https://gitlab.com/fedora/legal/fedora-license-data, using the license review template, similar to the one I linked. Fill in all the applicable fields.
2. Ideally, create the MR to update public-domain-text.txt yourself, referencing your work item and imitating https://gitlab.com/fedora/legal/fedora-license-data/-/merge_requests/838.
3. The dedication here is trivial, so you can expect that someone from Fedora Legal will mark it as approved within a couple of days or so.
4. Change the License field of this submission to LicenseRef-Fedora-Public-Domain (you already did this), and ideally add a comment linking your fedora-license-data issue so it’s easy to see that the text has been submitted for review.

----

Since you passed “-l” to %pyproject_save_files, asserting that a license file is properly marked in the dist-info metadata, you don’t need to also install an additional copy in /usr/share/licenses/…: you can remove “%license LICENSE”.

----

If you are going to do this:

  echo "This software is released into the public domain." > LICENSE

you should be able to cite where that exact dedication came from, either in the source code or in a PR that was merged upstream.

In fact, this text doesn’t seem to match what was added upstream in https://github.com/dahlia/iso4217/commit/aa87a85494ff240a3faadfaa84eb832b8168d4df. (On preview, I see that you addressed this in https://bugzilla.redhat.com/show_bug.cgi?id=2460052#c8, above.)

Consider something like this instead:

  # Add LICENSE file with Public Domain declaration
  Patch:          %{url}/commit/aa87a85494ff240a3faadfaa84eb832b8168d4df.patch

  […]

  %autosetup -n %{srcname}-%{version} -p1

----

A better source URL would be https://github.com/dahlia/iso4217/archive/%{version}/iso4217-%{version}.tar.gz, or %{url}/archive/%{version}/iso4217-%{version}.tar.gz if you prefer. That way, the archive name matches the extraction directory. At minimum, please use the %{version} macro instead of hard-coding the version number in the URL.

----

It seems like it ought to be possible to run the test suite. You should try to do this, https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_tests. If there’s something stopping you, please document it in a spec-file comment.

----

Putting the %check section after %files doesn’t make anything work differently, but it hurts legibility. Consider putting it after %install instead.

----

The build has to work offline. Upstream’s setup.py downloads an XML data file at build time and installs it into the source tree as iso4217/table.xml.

https://github.com/dahlia/iso4217/blob/7f4c46981f72b571f3f461b6e8d971db5a3e19ef/setup.py#L20-L52

This is the reason that the “Fedora Review Service” test build failed. You could probably work around this by including the data table as additional Source, manually copying it into the source tree in %prep, and exporting ISO4217_DOWNLOAD=0 before %pyproject_generate_buildrequires and before %pyproject_wheel.

There’s a bigger problem, though: it’s not clear what license applies to the XML data file, https://www.six-group.com/dam/download/financial-information/data-center/iso-currrency/lists/list-one.xml. I found the link to this data file at https://www.six-group.com/en/products-services/financial-information/market-reference-data/data-standards.html#scrollTo=currency-codes, but it doesn’t clarify the license terms. The corresponding ISO standard page, https://www.iso.org/iso-4217-currency-codes.html, doesn’t help either: “free of charge” is not a license. In my opinion, this will need to be clarified before the package can be approved.