https://bugzilla.redhat.com/show_bug.cgi?id=2250532 --- Comment #4 from Ben Beasley <code@xxxxxxxxxxxxxxxxxx> --- Thanks for the review! (In reply to Sandro from comment #3) > [x]: License file installed when any subpackage combination is installed. > > => mrs_tools-1.0.2-1.fc40.noarch.rpm does not contain a license file. I see > that it requires python3-nifti-mrs, which has a license file. > Personally, I'd err on the side of caution here. Not blocking this review, > but rather something to keep in mind. The guidelines are very explicit that a dependency on a subpackage with the correct license file is sufficient[1]. I agree it’s important to make sure we don’t break the dependency without handling the license file, but I don’t think it’s worth duplicating the license file, and I haven’t found doing so to be the usual practice. > [!]: Latest version is packaged. > > => Version 1.1.1 was released on December 6, 2023. Please update before > importing. Yeah, I should have double-checked this before calling for a reviewer. I’ll post an updated submission. The changes are not disruptive[2], so I don’t expect there will be anything different in the packaging. > => A nitpicky bit: Consider standardizing on BRs. You are using > `python3-foo` and `%{py3_dist foo}` intermixed. I agree with standardizing in general, but in this case there is a good reason for each python3-foo: - For python3-nifti-mrs, this is a fully-versioned dependency across subpackages of the same package in the manner of [3], so it should use the exact binary package name to avoid any ambiguity or versioning discrepancies (like with Python Provides dropping trailing .0’s, while they’re maintained in the RPM package version). - For python3-sphinx-latex, this is a metapackage that brings in the dependencies for building LaTeX with Sphinx; it does not contain any importable package and specifically does not Provide python3dist(sphinx-latex). So I’ve standardized on using %{py3_dist foo} everywhere that I can – for all external dependencies on importable Python packages - and this package just happens to contain only one such dependency, %{py3_dist pytest}, along with both of the notable exceptions where python3-foo doesn’t really mean %{py3_dist foo}. > => Bonus points for handcrafted man pages! Get your hand-crafted, artisanal man pages right here! Only fifteen dollars a sheaf! Join our mailing list and get 10% off! [1] https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#subpackage-licensing [2] https://github.com/wtclarke/nifti_mrs_tools/compare/1.0.2...1.1.1 [3] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package -- 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=2250532 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202250532%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
