https://bugzilla.redhat.com/show_bug.cgi?id=2427087
Fabio Valentini <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|[email protected] |[email protected] CC| |[email protected] Flags| |fedora-review? Status|NEW |ASSIGNED --- Comment #2 from Fabio Valentini <[email protected]> --- Initial review pass: 1. Weird rpm macros? > %bcond_without check > %global cargo_install_lib 0 > %global _cargo_generate_buildrequires 0 Not sure why / how you added these. The first one should be either `%bcond check 1` or dropped. The second one can be dropped if you replace `%cargo_install` with one manual `install` command. The third one looks very strange. see also https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_non_crate_rust_project 2. The License tag is incomplete. It currently only refers to "phrog" itself. You also need to take into account statically linked Rust dependencies (the summary is printed by `%cargo_license_summary` during the build, you should find it in the "build.log" file after a build (between "START LICENSE SUMMARY" and "END LICENSE SUMMARY"). 3. Do not use double-underscore-prefixed RPM macros. These are implementation details and should not be used in spec files. In this case, just use plain "install" instead of "%{__install}". 4. I would recommend to use "install ... target/rpm/phrog -t ..." instead of %cargo_install. In a simple case like this one, the latest version of the Packaging Guidelines recommend against using `%cargo_install` in favor of just installing the built executables yourself. see https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_non_crate_rust_project 5. Create files / folder structure necessary for tests in %check. Setting up the test environment should happen in %check. This way it cannot influence the built package, since the files are collected *before* %check is run. 6. Missing verification of the .desktop files. ".desktop" files MUST be validated with "desktop-file-validate" (from desktop-file-utils). see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_file_install_usage 7. Differentiate between folders and files in the %files list, and make sure all "intermediate" folders are accounted for. Right now ownership of folders / files is not entirely clear, and incomplete. For example, the /etc/phrog directory is "unowned" in the current setup. Looks like (taking "%{_sysconfdir}/phrog/autostart" as an example) you would need to do something like: %dir %{_sysconfdir}/phrog %dir %{_sysconfdir}/phrog/autostart (assuming /etc/phrog/autostart is currently an empty directory) If it has contents (why does the package install default config files in /etc instead of /usr?), then you'd need to add one more line: %config(noreplace) %{_sysconfdir}/phrog/autostart/* 8. Missing systemd scriptlets for the user session service. Both system and user session service units need scriptlets. -- 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=2427087 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202427087%23c2 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
