https://bugzilla.redhat.com/show_bug.cgi?id=2327650 --- Comment #19 from wojnilowicz <lukasz.wojnilowicz@xxxxxxxxx> --- (In reply to Fabio Valentini from comment #17) > I haven't forgotten this ticket - I'll come back to do another review round > ASAP. > I'm just working through the flood of emails that arrived during / after the > holidays, please bear with me. No problem. Thanks for the update. (In reply to Fabio Valentini from comment #18) > > > 3. You have disabled running tests. Why? Please document why you can't run > > > tests, or ideally, run at least *some* tests. > > A mistake due to the confusing %bcond_with behavior. > > Thanks! You should also be able to use `%bcond check 1` instead, which is > less confusing IMO. > This is the new default since rust2rpm v27, since this macro is now usable > in all branches of Fedora and on EPEL 9 and 10. Done. > > There is also https://github.com/ActivityWatch/aw-server which is Python > > based. > > The source package "aw-server-rust" has the binary "aw-server" (without the > > rust suffix) because aw-qt (UI for it) calls it that way. It can work with > > aw-server (Python based) and aw-server-rust (Rust based). I'm not sure if > > the Python based form would make it to Fedora, so if that's not a problem, > > then I would leave the names as they are right now. > > Makes sense to me. > > Is aw-sync specific to the Rust implementation? > Otherwise it would make sense to rename that one too. Indeed. It's even named aw-sync-rust at the project's page at https://github.com/ActivityWatch/aw-server-rust/tree/master/aw-sync > > > 7. You've included a systemd user session unit, but haven't added the > > > necessary scriptlets for it: > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/ > > > #_user_units > > Skipped initially, because I thought I don't need them. I'm still not sure > > of their effect. > > The macros restart the service for all logged-in users (that have the > service enabled) when the package is updated. > If "online restart" is not supported by the service, then use the postun > macro without restart instead. Thanks for the explanation. I think, that restart is OK. > A few more minor things: > > 1. Version: 0.13.1^20241216.%{short_commit} > > I usually recommend to add "git" to prefix separate the shortcommit, which > would look like > > Version: 0.13.1^20241216.git%{short_commit} > > see also > https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ > #_snapshots > > But this is optional. Done. > 2. The License tag is more complicated than it needs to be. > It's not *wrong*, but you could shorten it by applying commutativity and > associativity for the AND and OR operators, and dropping duplicate clauses. I think it's done now. > 3. Would it be possible to upstream the patches for fern v0.6 -> v0.7, > fancy-regex v0.12 -> v0.13, and rusqlite v0.30 -> v0.31 version bumps? Naturally. I just wait for this review to finish. > 4. You probably shouldn't modify the package.version in Cargo.toml: > > > # append current commit to the version string > > sed -ri 's/version = ("[[:alnum:]]+\.[[:alnum:]]+\.[[:alnum:]]+)/version = \1+%{short_commit}/' aw-server/Cargo.toml > > This is not necessary. Or is this version number shown in the UI somewhere, > and you'd like it to show that the package is built from a git snapshot? Exactly. That's shown in the UI, and the upstream does the same. I put more verbose comment to that line. > 5. Do you think the manual pages for aw-server / aw-sync are useful? I > usually dislike just piping "--help" output through help2man. But if you > think they are useful, then keep things as they are. No. Just following the guideline at https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages On the other hand I like to view things through man. Is it OK now? Spec URL: https://wojnilowicz.fedorapeople.org/aw-server-rust.spec SRPM URL: https://wojnilowicz.fedorapeople.org/aw-server-rust-0.13.1^20241216.gita0cdef9-1.fc41.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=2327650 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202327650%23c19 -- _______________________________________________ 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
