https://bugzilla.redhat.com/show_bug.cgi?id=2427087
--- Comment #4 from Sam Day <[email protected]> --- Thanks for the fab review, Fab! > 1. Weird rpm macros? ... Not sure why / how you added these. Hehe okay so the _cargo_generate_buildrequires one is certainly odd. I can't remember where I found that (an LLM hallucination, most likely). I disagree on the other two macros being "weird", though: https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject.org/+bcond_without&patternType=regexp&sm=0 https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject.org/+cargo_install_lib&patternType=regexp&sm=0 Anyway, I've changed the preamble to suit your preference and dropped the _cargo_generate_buildrequires thingie. > 2. The License tag is incomplete. Fixed. Is there any tooling that will detect if this falls out of sync, though? Presumably this needs to be checked every time there's a new release with changed deps. > 3. Do not use double-underscore-prefixed RPM macros. Out of interest, are there guidelines somewhere expressly forbidding this? I see a lot of prior art making use of this macro: https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject.org/+__install&patternType=regexp&sm=0 To expedite the review I've made the requested change anyways. > 4. I would recommend to use "install ... target/rpm/phrog -t ..." instead of > %cargo_install. Done. > 5. Create files / folder structure necessary for tests in %check. I assume you just meant the `XDG_RUNTIME_DIR` creation? I don't see how creating something in `/tmp` will affect anything in `%{_buildroot}`, but I've made the requested change anyway as it's harmless (and more coherent I suppose) > 6. Missing verification of the .desktop files. This is still not yet possible as desktop-file-validate doesn't have a tagged release with Phosh support: https://gitlab.freedesktop.org/xdg/desktop-file-utils/-/merge_requests/24 > (why does the package install default config files in /etc instead of /usr?) I assume you're referring to greetd-config.toml? That file is the default config that users are expected/welcome to modify (hence `%config(noreplace)`) if users choose to use `phrog.service` (rather than manually wiring up `greetd.service` themselves). > 8. Missing systemd scriptlets for the user session service. Oops apologies, LLM *and* f-r-s review already warned me of that and I forgot to fix it (╯°□°）╯︵ ┻━┻ > 9.: Please "version" the vendor tarball. Done. --- Spec URL: https://samcday.fedorapeople.org/phrog.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/5945/140805945/phrog-0.50.0-1.fc44.src.rpm -- 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%23c4 -- _______________________________________________ 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
