Spec URL: https://alciregi.fedorapeople.org/rustrpm/rust-cfonts.spec SRPM URL: https://alciregi.fedorapeople.org/rustrpm/rust-cfonts-1.1.0-1.fc37.src.rpm Description: This is a silly little command line tool for sexy ANSI fonts in the console. Give your cli some love. Fedora Account System Username: alciregi Successful koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=94784705
Two Quick Comments: It looks like you're adding a license file from upstream to the SRPM file, but then do nothing with it? You will probably want to do something like "cp -pav %{SOURCE1} ." at the end of %prep, and then use that file (i.e. replace the "# FIXME: no license file detected" warnings in both file lists - in the case of the subpackage for the actual binary, with `%license LICENSE`, and in the case of the "-devel" subpackage, with `%license %{crate_instdir}/LICENSE`. You will also need to determine the license of everything that's statically linked into your binaries. You can use the %cargo_license / %cargo_license_summary macros for this, i.e. adding something like this after %cargo_build: %cargo_license_summary %{cargo_license} > LICENSE.dependencies The first one prints a summary to the build log (which you can use for determining the License tag of the "-n %{crate}" subpackage), the second one writes a file into the builddir that contains a complete license breakdown for all statically linked components (which you can include with %license LICENSE.dependencies) in the subpackage for the binary, as well. You can take a look at rpm-sequoia for an example of how to handle this: https://src.fedoraproject.org/rpms/rust-rpm-sequoia/blob/rawhide/f/rust-rpm-sequoia.spec
Spec URL: https://alciregi.fedorapeople.org/rustrpm/rust-cfonts.spec SRPM URL: https://alciregi.fedorapeople.org/rustrpm/rust-cfonts-1.1.0-1.fc37.src.rpm Thank you. I hope it is ok.
Copr build: https://copr.fedorainfracloud.org/coprs/build/5537301 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2149953-rust-cfonts/fedora-rawhide-x86_64/05537301-rust-cfonts/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.
Taking this review Some comments / issues with the package: - The upstream project does not include a license file in `Cargo.toml`. I can see that you add it manually but it would be nice if you could push the change to upstream. Maybe by specifying `license-file` [1]?. - The license path for the `devel` package is wrong. It should be `%license %{crate_instdir}/LICENSE`. - `rust2rpm` always adds the `%doc` directive to `devel` packages. I am not sure if this is actually required as I couldn't find anything about it in the guidelines but I think adding it shouldn't hurt. Something like `%doc %{crate_instdir}/README.md` [1] https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields
(In reply to blinxen from comment #4) > Taking this review > > Some comments / issues with the package: > > - The upstream project does not include a license file in `Cargo.toml`. I > can see that you add it manually but it would be nice if you could push the > change to upstream. Maybe by specifying `license-file` [1]?. No. The `license-file` field in Cargo.toml is supposed to only be used if the license is non-standard (i.e. custom license with no corresponding SPDX identifier). > - The license path for the `devel` package is wrong. It should be `%license > %{crate_instdir}/LICENSE`. That *would* be true, but the upstream Cargo.toml specifies files to include, and `LICENSE` is not among the listed files. So it cannot be referenced this way unless the `include` setting in Cargo.toml is also fixed to include the "LICENSE" file. Otherwise it will not get copied to %{crate_instdir}. > - `rust2rpm` always adds the `%doc` directive to `devel` packages. I am not > sure if this is actually required as I couldn't find anything about it in > the guidelines but I think adding it shouldn't hurt. Something like `%doc > %{crate_instdir}/README.md` Yes, this line seems to be missing from the -devel subpackage's files: %doc %{crate_instdir}/README.md
Sorry. I missed a piece. Apart adding %doc %{crate_instdir}/README.md, should we ask upstream to fix or enhance something in the Cargo.toml file in order to avoid to manually include the LICENSE file? Thanks.
> Apart adding %doc %{crate_instdir}/README.md, should we ask upstream to fix or enhance something in the Cargo.toml file in order to avoid to manually include the LICENSE file? Yes. They are distributing sources that don't include their own license file. :( The list of files here needs to include a "LICENSE" file: https://github.com/dominikwilkowski/cfonts/blob/released/rust/Cargo.toml#L14 And there needs to be a copy (or symlink) of the file in the "rust" directory. Please report this with the upstream project. Also note that while the project's Cargo.toml references "../README.md" as its "readme" file, this is not really defined behaviour unless the "README.md" file is also included in the list of files to "include". --- Additionally, the "%package -n %{crate}" subpackage is still missing a License tag that contains the licenses that are printed by %cargo_license_summary.
Ok. I will contact upstream. About missing License tag. Doesn't %license LICENSE.dependencies where LICENSE.dependencies is generated by %{cargo_license} is sufficient?
(In reply to Alessio from comment #8) > Ok. I will contact upstream. Thanks! > About missing License tag. Doesn't > %license LICENSE.dependencies > where LICENSE.dependencies is generated by %{cargo_license} > is sufficient? No: https://docs.fedoraproject.org/en-US/legal/license-field/#_rust_packages
Ah. So the License tag of the spec file, should be License: Apache-2.0 AND GPL-3.0-or-later AND ISC AND MIT considering that # Apache-2.0 # Apache-2.0 OR BSL-1.0 # GPL-3.0-or-later # ISC # MIT # MIT OR Apache-2.0 is the output of %cargo_license_summary. Or should it be License: Apache-2.0 AND (Apache-2.0 OR BSL-1.0) AND GPL-3.0-or-later AND ISC AND MIT AND (MIT OR Apache-2.0) Thank you.
According to the latest guidance from Red Hat legal, the latter is the preferred form. c.f. https://docs.fedoraproject.org/en-US/legal/license-field/#_conjunctive_and_licensing and https://docs.fedoraproject.org/en-US/legal/license-field/#_combined_disjunctive_and_conjunctive_license_expressions
Are you still interested in this package?
(In reply to Fabio Valentini from comment #12) > Are you still interested in this package? I would be interested, but I fear that I lack some concepts and knowledge :-/
The spec does look good to me, with the exception of the missing license tag + breakdown for the binary subpackage. Adding """ # Apache-2.0 # Apache-2.0 OR BSL-1.0 # GPL-3.0-or-later # ISC # MIT # MIT OR Apache-2.0 License: Apache-2.0 AND (Apache-2.0 OR BSL-1.0) AND GPL-3.0-or-later AND ISC AND MIT AND (MIT OR Apache-2.0) """ as discussed in https://bugzilla.redhat.com/show_bug.cgi?id=2149953#c10 is the only thing that's still missing, as far as I can see. === The only other problem is that the current version (cfonts 1.1.0) has an outdated dependency and doesn't build right now. The suppors-color crate has been updated to version 2, which cfonts is not compatible with yet. You might want to ask upstream about bumping the supports-color dependency (if possible). I also commented on the upstream ticket that you raised, hopefully I could clear some things up.
Thank you @decathorpe