https://bugzilla.redhat.com/show_bug.cgi?id=2392155 --- Comment #4 from wojnilowicz <lukasz.wojnilowicz@xxxxxxxxx> --- Thanks for the thorough review and examples for the toml file. I interleaved my replies with yours down below. (In reply to Ben Beasley from comment #2) > This package looks pretty good, overall! There are a few things I think you > should consider before I approve it. > > Package Review > ============== > > Legend: > [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated > > The spec file is generated by rust2rpm, simplifying the review. There is a > cargo metadata package, which is reasonable and properly documented, and the > License expression is constructed from %cargo_license_summary output. This > part > doesn’t look quite right; see Issues, below. > > > Issues: > ======= > - Package does not contain duplicates in %files. > Note: warning: File listed twice: > /usr/share/cargo/registry/wiremix-0.7.0/LICENSE-APACHE > See: https://docs.fedoraproject.org/en-US/packaging- > guidelines/#_duplicate_files > > This is not a serious defect, and is due to reasonable design decisions in > rust2rpm. Fine. I guess I shouldn't do anything about it. > - The output of %cargo_license_summary is correctly pasted into the spec > file: > > # (MIT OR Apache-2.0) AND Unicode-DFS-2016 > # Apache-2.0 OR BSL-1.0 > # Apache-2.0 OR MIT > # Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT > # MIT > # MIT OR Apache-2.0 > # Unlicense OR MIT > # Zlib > > However, the license expression isn’t correctly constructed. Output of the > form > > # A OR B > # C OR D > > corresponds to an SPDX expression of the form > > (A OR B) AND (C OR D) > > and cannot be smashed together without parentheses. You also *don’t* need > parentheses around a license with an exception: > > # E WITH F-exception > # G > > is > > E WITH F-exception AND G > > not > > (E WITH F-exception) AND G > > which is still technically correct, but has unnecessary parentheses. > > For more details, see > https://docs.fedoraproject.org/en-US/legal/license-field/. Note that there > is > a rule that allows you to simplify "(A OR B) AND A AND B" to just "A AND > B"; > I don’t like this rule because I think it adds unnecessary complexity and > reduces clarity, and you are not required to do the simplification, but I > do > have to note that it is permitted. > > While it isn’t explicitly stated above, you can simplify > "(A OR B) AND (B OR A)" into "A OR B" (order isn’t important). > > So, instead of this: > > License: MIT AND Apache-2.0 AND Unicode-DFS-2016 AND Apache-2.0 > OR BSL-1.0 AND (Apache-2.0 WITH LLVM-exception) AND Unlicense AND Zlib > > try something like this: > > # (MIT OR Apache-2.0) AND Unicode-DFS-2016 > # Apache-2.0 OR BSL-1.0 > # Apache-2.0 OR MIT > # Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT > # MIT > # MIT OR Apache-2.0 > # Unlicense OR MIT > # Zlib > License: %{shrink: > (MIT OR Apache-2.0) AND > MIT AND > Unicode-DFS-2016 AND > Zlib AND > (Apache-2.0 OR BSL-1.0) AND > (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND > (Unlicense OR MIT) > } > > There is no particular prescribed order of the terms in the License field; > I > am following Fabio Valentini’s usual convention of putting the crate’s own > license first, then terms consisting of single licenses in alphabetical > order, then disjunctive subexpressions (… OR …) in alphabetical order > without > sorting the terms within the disjunctionve subexpressions. Other > conventions > are possible and reasonable. Having some sort of consistent convention > makes > it easier for you to audit the expression when the %cargo_license_summary > output changes. > > Similarly, using the %{shrink:…} macro to write one term per line makes it > much easier to edit and audit the License expression. I see my error. I like the construct with shrink better - less processing for me. Thanks. > - Since the wiremix crate is not currently designed to be usable as a > library, > https://github.com/tsowell/wiremix/issues/13, > https://crates.io/crates/wiremix/reverse_dependencies, you might consider > omitting the Rust crate library subpackages since they are unnecessary. You > can handle this via a simple configuration option in rust2rpm.toml: > > [package] > cargo-toml-patch-comments = [ > "Missing dependency crossterm 0.29.0 but works with 0.28.0.", > "Build-only depencency not needed ro run wiremix.", > ] > cargo-install-lib = false Added. > - It would make sense to include the sample user configuraton file > wiremix.toml > as documentation. You can also do this in rust2rpm.toml, by adding the > following to the [package] section: > > doc-files.include = [ > "wiremix.toml", > ] Added. > - While a man page is not required, it is always desired for a CLI/TUI tool. > > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages > > wiremix.aarch64: W: no-manual-page-for-binary wiremix > > If you like, you could try working with upstream to supply a man page. The > https://github.com/oxipng/oxipng project is a pretty good example of how a > man page can be automatically generated using clap_mangen, > https://crates.io/crates/clap_mangen, via an xtask. > > You could also generate a man page downstream using help2man, which > produces > a pretty good result for this particular tool. You can add the following > sections to rust2rpm.toml: > > [requires] > build = ["help2man"] > > [scripts.build] > post = ["help2man --no-info --output=wiremix.1 target/rpm/wiremix"] > > [scripts.install] > post = ["install -t %{buildroot}%{_mandir}/man1 -D -p -m 0644 wiremix.1"] > > and then add to the [package] section: > > extra-files = ["%{_mandir}/man1/wiremix.1*"] > > Neither approach is required for approval, but either would be helpful to > users. I don't want to work with upstream. The generated man page is good enough for me, and no one knows how much short-lived project wiremix will be. Thanks for the examples and reference thought. Is it good enough to be approved now? rust2rpm.toml: https://wojnilowicz.fedorapeople.org/rust2rpm.toml [fedora-review-service-build] -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2392155 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202392155%23c4 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
