https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #19 from wojnilowicz lukasz.wojnilowicz@gmail.com --- (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)
- 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
- 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:
- 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.
- 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.
- 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.
- 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.
- 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%5E20241216.gita0c...