https://bugzilla.redhat.com/show_bug.cgi?id=2427087
--- Comment #9 from Fabio Valentini <[email protected]> --- (In reply to Sam Day from comment #4) > Thanks for the fab review, Fab! Ha. :) Thanks for the update - will take another look. ================================================================================ ad 1: "weird" was probably the wrong word here, sorry. The only one that "weird" applied to was the hallucinated one, the others are just outdated, not weird. As for the other two: I would recommend to use `%bcond check 1` instead of `%bcond_without check`. They mean the same thing, but the former is the "modern" one and is usually considered less confusing (yes, "%bcond_without check" means "with checks by default" ...). The other one ("%global cargo_install_lib 0") was dropped from the Rust Packaging Guidelines for packages like this one, and is unused if you don't use `%cargo_install` (which is also in line with the latest Rust guidelines). -> https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_non_crate_rust_project ================================================================================ ad 2) > 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. No, there is no tooling for this currently. Verifying that the License tag is correct at build-time would cause a random amount of spurious build failures over time whenever the dependency tree of a project (even through transitive dependencies) changes in any way. The "new" Go packaging tooling *can* do this because there, dependencies are *always* vendored, so there is no dependency drift. For Rust packages, usually the license tag is checked and updated when doing package updates and / or other packaging changes. ================================================================================ ad 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: Actually, yes. I couldn't find the reference myself, but after asking for help, we found it: ``` Macros with names beginning with underscores are generally considered to be implementation details internal to RPM and its associated macro packages and SHOULD NOT be referenced in specfiles except to set their values in order to influence RPM behavior. This implies that macro forms of system executables SHOULD NOT be used. For example, rm should be used in preference to %{__rm}. ``` -> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros So you *can* use them if you really need to, but you shouldn't if you don't. > https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject. > org/+__install&patternType=regexp&sm=0 Yes, this is / was a common pattern in really old packages. ;) > To expedite the review I've made the requested change anyways. Thanks! ================================================================================ > > 4. I would recommend to use "install ... target/rpm/phrog -t ..." instead > > of %cargo_install. > > Done. 👍 (see also point 1. / Rust package template for packages like this) ================================================================================ > > 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) Yes, that's what I meant. In this case it *shouldn't* make a difference, but in general, this is the safe way to set up a test environment that *cannot* influence the package output. ================================================================================ > > 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 -> "I've just added the `desktop-file-validate` invocation to `%check` in my local tree." This is fine. 👍 > > (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). Yes, that's what I was referring to. It's not the most "modern" ("systemd style") way to handle config files, but it should be fine. ================================================================================ > > 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 (╯°□°）╯︵ ┻━┻ -> But actually, it turns out we don't want to (and kinda can't) add these Right - I forgot about the case where the .service files aren't actually enable-able by systemd. Thank you for filing a PR to clarify the packaging guidelines for this case, this should be fine the way it is then. ================================================================================ > > 9.: Please "version" the vendor tarball. > > Done. 👍 ================================================================================ Will do a full review pass ASAP. -- 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=2427087 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202427087%23c9 -- _______________________________________________ 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
