https://bugzilla.redhat.com/show_bug.cgi?id=2335285 --- Comment #10 from Fabio Valentini <decathorpe@xxxxxxxxx> --- First pass review (sorry for the delay): 1. You can simplify the Source0 line by reusing URL (also, use the standard format documented here): https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags """ Source0: %{url}/archive/%{version}/selenium-%{version}.tar.gz """ 2. This comment is not really accurate: > Both patches are trying to circumvent generic build requirements which are currently not usable in fedora The problem is that the application doesn't declare those dependencies or code paths as OS specific, so the dependencies are unconditionally pulled in even when they're unnecessary. The patches looks correct though. For the update of the "zip" crate from version 0.6 to 2.x, that will likely happen soon in Fedora. And an update for "which" from v6 to v7 should be acceptable for the upstream project. 3. The BuildRequires are all (but one) redundant: > BuildRequires: rust > BuildRequires: cargo > BuildRequires: rust-packaging > BuildRequires: cargo-rpm-macros >= 24 rust is pulled in by cargo. cargo is pulled in by cargo-rpm-macros. rust-packaging is an alias for cargo-rpm-macros that is only provided for backwards compatibility. Please drop all but "cargo-rpm-macros >= 24". 4. The summary is repetitive. It should not just repeat the package name. In this case I would suggest: """ Summary: Automated driver and browser management for Selenium """ 5. This is wrong (and weirdly placed): > %define cargo_install_lib 0 Please move this to the top of the spec file, and replace %define with %global. The former should basically never be used, unless you *really* know that you *need* it. 6. If the project doesn't support running *any* tests without internet access, then just remove this: > #Project doesn't support offline tests > #%%check > #%%cargo_test Keeping it commented out doesn't provide any value IMO. 7. You need to update the License tag with the statically linked Rust dependencies. Adding these two macros at the end of the %build scriptlet would be the first step (to generate the list of statically linked dependencies): """ %{cargo_license_summary} %{cargo_license} > LICENSE.dependencies """ Then you can include the written file with "%license LICENSE.dependencies" in %files, and adapt the License tag for the license summary written to the build.log during the build. Side note: It looks like this project provides implementations of "selenium-manager" in multiple programming languages ... I suppose it would only ever make sense to provide a Fedora package for *one* of those implementations (in this case, the one in Rust)? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2335285 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202335285%23c10 -- _______________________________________________ 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
