https://bugzilla.redhat.com/show_bug.cgi?id=2327650 --- Comment #18 from Fabio Valentini <decathorpe@xxxxxxxxx> --- > > 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. > 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. > > 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. > > 8. Patch 1 looks strange. Is it OK to just remove the openssl crate > > dependency entirely? I would have just removed the "vendored" feature from > > it. I wonder why this still compiles with the dependency removed entirely > > ... > No mention of openssl usage in aw-sync. It wasn't there initially at all, > and isn't present for other platforms. It appeared with the bug at > https://github.com/ActivityWatch/aw-server-rust/issues/478 > The app is distributed as AppImage, and I believe that openssl is included > for some side effect that it gives, and that makes this distribution form > working. This sounds like they depend on the openssl crate only transitively, and need to force it to build vendored OpenSSL and statically link it. This is not necessary for an RPM package, so in that case it's OK to remove the dependency. > > 10. The aw-webui subdirectory is empty (which might explain why things fail) > > - it's an uninitialized git submodule. I suspect that you will actually need > > to include those sources separately. > This is expected. I already include those sources separately from > nodejs-aw-webui and point to them through > export AW_WEBUI_DIR="%{_datadir}/aw-webui/dist/" Ah, great, I missed this! Looks good to me then. 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. 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. 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? 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? 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. -- 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%23c18 -- _______________________________________________ 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
