[Bug 2327650] New: Review Request: aw-server-rust - A re-implementation of aw-server in Rust
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Bug ID: 2327650 Summary: Review Request: aw-server-rust - A re-implementation of aw-server in Rust Product: Fedora Version: rawhide Status: NEW Component: Package Review Assignee: nobody@fedoraproject.org Reporter: lukasz.wojnilowicz@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://wojnilowicz.fedorapeople.org/aw-server-rust.spec SRPM URL: https://wojnilowicz.fedorapeople.org/aw-server-rust-0.13.1%5E20241109.a0cdef...
Description: A re-implementation of aw-server in Rust
Fedora Account System Username: wojnilowicz
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/Activity | |Watch/%{name}
--- Comment #1 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/8287138 (failed)
Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please make sure the package builds successfully at least for Fedora Rawhide.
- If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
wojnilowicz lukasz.wojnilowicz@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value
--- Comment #2 from wojnilowicz lukasz.wojnilowicz@gmail.com --- [fedora-review-service-build]
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #3 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/8338770 (failed)
Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please make sure the package builds successfully at least for Fedora Rawhide.
- If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review? CC| |decathorpe@gmail.com Assignee|nobody@fedoraproject.org |decathorpe@gmail.com
--- Comment #4 from Fabio Valentini decathorpe@gmail.com --- This package fails to build because it depends on fern v0.6 with the "colored" feature. Only fern v0.7 has this feature in Fedora.
Some other notes:
# drop an unused, benchmark-only criterion dev-dependency to speed up builds find -name "*.toml" | xargs sed -ri '/^[[:blank:]]*criterion\b/d'
# switch to SQLite available in Fedora sed -ri 's/rusqlite = { version = "0.30", features = ["chrono", "serde_json", "bundled"] }/rusqlite = { version = "0.31", features = ["chrono", "serde_json"] }/' aw-datastore/Cargo.toml
# switch to fancy-regex available in Fedora sed -ri 's/fancy-regex = "0.12.0"/fancy-regex = "0.13.0"/' aw-query/Cargo.toml aw-transform/Cargo.toml
# append current commit to the version string sed -ri 's/version = "0.13.1"/version = "0.13.1+%{short_commit}"/' aw-server/Cargo.toml
# remove Android dependencies sed -ri '/target_os="android"/,+4d' aw-server/Cargo.toml
# remove a dependency needed only for the vendored package sed -ri '/target_os="linux"/,/openssl/d' aw-sync/Cargo.toml
# jemallocator will not be packaged for Fedora, so remove it sed -ri '/target_os="linux", target_arch="x86"/,+1d' aw-server/Cargo.toml sed -ri '/target_os = "linux", target_arch = "x86"/,/static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc;/d' aw-server/src/main.rs
Please, don't use `sed` for changing Cargo.toml. It is really hard to understand and debug. A patch would be much simpler and easier to maintain.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #5 from wojnilowicz lukasz.wojnilowicz@gmail.com --- (In reply to Fabio Valentini from comment #4)
This package fails to build because it depends on fern v0.6 with the "colored" feature. Only fern v0.7 has this feature in Fedora.
Indeed. A sed line changing to v0.7 supposed to be there. I'll fix this based on your stance on sed lines.
Some other notes:
# drop an unused, benchmark-only criterion dev-dependency to speed up builds find -name "*.toml" | xargs sed -ri '/^[[:blank:]]*criterion\b/d'
# switch to SQLite available in Fedora sed -ri 's/rusqlite = { version = "0.30", features = ["chrono", "serde_json", "bundled"] }/rusqlite = { version = "0.31", features = ["chrono", "serde_json"] }/' aw-datastore/Cargo.toml
# switch to fancy-regex available in Fedora sed -ri 's/fancy-regex = "0.12.0"/fancy-regex = "0.13.0"/' aw-query/Cargo.toml aw-transform/Cargo.toml
# append current commit to the version string sed -ri 's/version = "0.13.1"/version = "0.13.1+%{short_commit}"/' aw-server/Cargo.toml
# remove Android dependencies sed -ri '/target_os="android"/,+4d' aw-server/Cargo.toml
# remove a dependency needed only for the vendored package sed -ri '/target_os="linux"/,/openssl/d' aw-sync/Cargo.toml
# jemallocator will not be packaged for Fedora, so remove it sed -ri '/target_os="linux", target_arch="x86"/,+1d' aw-server/Cargo.toml sed -ri '/target_os = "linux", target_arch = "x86"/,/static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc;/d' aw-server/src/main.rs
Please, don't use `sed` for changing Cargo.toml. It is really hard to understand and debug. A patch would be much simpler and easier to maintain.
Could you reconsider letting me keep the sed lines based on the fact that: 1) this is a commit based package, which means it could change frequently from commit to commit 2) the source (aw-server-rust) and the target (fedora) have lots of dependencies that need to match and I believe they'll be changing frequently as well 3) there is also an aw-awatcher package (which I would like to send after this one) which updates dependencies like hell, although old ones work fine, and there I would also like to use sed. A draft spec, where this can be seen, at https://github.com/wojnilowicz/activitywatch-copr/blob/main/aw-awatcher.spec... ?
On the contrary, I see that it's easier for me to maintain sed lines in this case, than have to unpack the source, deal with merge conflicts and generate a new patch at each new commit of this package.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
--- Comment #6 from Fabio Valentini decathorpe@gmail.com --- I would prefer if you did not use sed to patch Cargo.toml.
It makes it really hard to debug ("did this sed actually do anything?") and makes it hard for other people to contribute to the package. Though I think there is a mode for sed that makes it fail if the expression doesn't match any line in the file? Using this would make this at least a bit better.
This is not a package for a "crate" so you don't necessarily need to re-generate the spec file with rust2rpm for every version, and most Cargo.toml patches won't affect the generated spec file. So if you make sed calls that don't make any changes fail, I think that would be "ok" (though I would still prefer using a patchset, similar to what we do in ruff, uv, or maturin).
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #8 from wojnilowicz lukasz.wojnilowicz@gmail.com --- (In reply to Fabio Valentini from comment #6)
I would prefer if you did not use sed to patch Cargo.toml.
It makes it really hard to debug ("did this sed actually do anything?") and makes it hard for other people to contribute to the package. Though I think there is a mode for sed that makes it fail if the expression doesn't match any line in the file? Using this would make this at least a bit better.
This is not a package for a "crate" so you don't necessarily need to re-generate the spec file with rust2rpm for every version, and most Cargo.toml patches won't affect the generated spec file. So if you make sed calls that don't make any changes fail, I think that would be "ok" (though I would still prefer using a patchset, similar to what we do in ruff, uv, or maturin).
I went with the patches approach, except the sed line that changes the version string. Alas "sed -ri s/source/target/;t;q1" didn't work for me in my SPEC file, so no robustness on this approach. I hope, you'll be fine with this though, as this is a visual change only.
(In reply to Fabio Valentini from comment #7)
On the contrary, I see that it's easier for me to maintain sed lines in this case, than have to unpack the source, deal with merge conflicts and generate a new patch at each new commit of this package.
Side note: You really *should* unpack the sources and do at least a quick check that new versions didn't add any objectionable content (non-redistributable files, etc.) anyway :)
Right :) I follow the project, and do so by monitoring the commits a bit. Nothing too exciting so far.
Anyway, I updated the SPEC file under the same link. Please review again.
[fedora-review-service-build]
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #9 from wojnilowicz lukasz.wojnilowicz@gmail.com --- [fedora-review-service-build]
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #10 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/8371511 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Found issues:
- Systemd user unit service file(s) in aw-server-rust Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_...
Please know that there can be false-positives.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #11 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/8371527 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Found issues:
- Systemd user unit service file(s) in aw-server-rust Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_...
Please know that there can be false-positives.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #12 from Fabio Valentini decathorpe@gmail.com --- Some comments:
1. You don't need to put the whole output of %cargo_license into the spec file, that is what the output of %cargo_license_summary is for.
2. The output of %cargo_license should be written to a file, and that file included in the package. (rust2rpm should do it this way?)
3. You have disabled running tests. Why? Please document why you can't run tests, or ideally, run at least *some* tests.
4. The package name is a bit strange. The executable is just "aw-server", you could just name the package "aw-server" too, it wouldn't conflict with any existing package. (and it would also mirror aw-sync executable being in the aw-sync package). If you want the source package name to remain "aw-server-rust" to match the upstream project, that's fine, but I would recommend to make the built packages just have the names "aw-server" and "aw-sync".
5. The order of sections in the spec file is very unusual - I would recommend to do <packages> <scriptlets> <file lists>.
6. This is more complicated than it needs to be:
install -Dm 0755 %{_builddir}/%{name}-%{commit}/target/rpm/{aw-server,aw-sync} -t %{buildroot}%{_bindir}
Just do
install -Dm 0755 target/rpm/{aw-server,aw-sync} -t %{buildroot}%{_bindir}
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_...
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 ...
9. In general, I would recommend to split patches into logical units, not split by which file they touch.
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #13 from wojnilowicz lukasz.wojnilowicz@gmail.com --- (In reply to Fabio Valentini from comment #12)
Some comments:
- You don't need to put the whole output of %cargo_license into the spec
file, that is what the output of %cargo_license_summary is for.
Done.
- The output of %cargo_license should be written to a file, and that file
included in the package. (rust2rpm should do it this way?)
Done.
- 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.
- The package name is a bit strange. The executable is just "aw-server",
you could just name the package "aw-server" too, it wouldn't conflict with any existing package. (and it would also mirror aw-sync executable being in the aw-sync package). If you want the source package name to remain "aw-server-rust" to match the upstream project, that's fine, but I would recommend to make the built packages just have the names "aw-server" and "aw-sync".
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.
- The order of sections in the spec file is very unusual - I would
recommend to do <packages> <scriptlets> <file lists>.
I believe the order is good now.
- This is more complicated than it needs to be:
install -Dm 0755 %{_builddir}/%{name}-%{commit}/target/rpm/{aw-server,aw-sync} -t %{buildroot}%{_bindir}
Just do
install -Dm 0755 target/rpm/{aw-server,aw-sync} -t %{buildroot}%{_bindir}
Done.
- 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.
- 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.
- In general, I would recommend to split patches into logical units, not
split by which file they touch.
Done.
- 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/"
Spec URL: https://wojnilowicz.fedorapeople.org/aw-server-rust.spec SRPM URL: https://wojnilowicz.fedorapeople.org/aw-server-rust-0.13.1%5E20241216.a0cdef...
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #14 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2062702 --> https://bugzilla.redhat.com/attachment.cgi?id=2062702&action=edit The .spec file difference from Copr build 8371527 to 8398295
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #15 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/8398295 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Found issues:
- Systemd user unit service file(s) in aw-server-rust Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_...
Please know that there can be false-positives.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #16 from wojnilowicz lukasz.wojnilowicz@gmail.com --- ping
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
wojnilowicz lukasz.wojnilowicz@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(decathorpe@gmail. | |com)
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(decathorpe@gmail. | |com) |
--- Comment #17 from Fabio Valentini decathorpe@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #18 from Fabio Valentini decathorpe@gmail.com ---
- 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.
- 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.
- 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.
- 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/#_snaps...
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.
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...
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #20 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2065336 --> https://bugzilla.redhat.com/attachment.cgi?id=2065336&action=edit The .spec file difference from Copr build 8398295 to 8495184
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #21 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/8495184 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Found issues:
- Systemd user unit service file(s) in aw-server-rust Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_...
Please know that there can be false-positives.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #22 from wojnilowicz lukasz.wojnilowicz@gmail.com --- ping
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #23 from Fabio Valentini decathorpe@gmail.com ---
- 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.
Now you simplified too much :D
This is what I would end up with as the simplest possible License tag:
""" License: MPL-2.0 AND Apache-2.0 AND BSD-3-Clause AND MIT AND Unicode-DFS-2016 AND (0BSD OR MIT OR Apache-2.0) AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR ISC OR MIT) AND (Apache-2.0 OR MIT) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Apache-2.0 OR Zlib) AND (Unlicense OR MIT) """
(deduplicated and sorted alphabetically, except for MPL-2.0, which is the license for aw-server-rust itself)
Other than that, looks good to me now, thank you!
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #24 from wojnilowicz lukasz.wojnilowicz@gmail.com --- (In reply to Fabio Valentini from comment #23)
- 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.
Now you simplified too much :D
This is what I would end up with as the simplest possible License tag:
""" License: MPL-2.0 AND Apache-2.0 AND BSD-3-Clause AND MIT AND Unicode-DFS-2016 AND (0BSD OR MIT OR Apache-2.0) AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR ISC OR MIT) AND (Apache-2.0 OR MIT) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Apache-2.0 OR Zlib) AND (Unlicense OR MIT) """
(deduplicated and sorted alphabetically, except for MPL-2.0, which is the license for aw-server-rust itself)
Other than that, looks good to me now, thank you!
Thanks. I feel, I would not get that easy to that.
I've just uploaded your change. 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...
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #25 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2067005 --> https://bugzilla.redhat.com/attachment.cgi?id=2067005&action=edit The .spec file difference from Copr build 8495184 to 8558148
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #26 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/8558148 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Found issues:
- Systemd user unit service file(s) in aw-server-rust Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_...
Please know that there can be false-positives.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+ Status|ASSIGNED |POST
--- Comment #27 from Fabio Valentini decathorpe@gmail.com --- (In reply to wojnilowicz from comment #24)
Thanks. I feel, I would not get that easy to that.
True 😓
It would be good to have a tool to do this automatically. I filed an RFE for it: https://pagure.io/fedora-rust/rust2rpm/issue/306
I've just uploaded your change. 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. gita0cdef9-1.fc41.src.rpm
Looks good to me!
======================================================================
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues: ======= - systemd_user_post is invoked in %post and systemd_user_preun in %preun for Systemd user units service files. Note: Systemd user unit service file(s) in aw-server-rust See: https://docs.fedoraproject.org/en-US/packaging- guidelines/Scriptlets/#_user_units
-> This seems to be a false positive. The scriptlets and macro calls are present.
===== MUST items =====
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [x]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: No rpmlint messages. [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: The License field must be a valid SPDX expression. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 2884 bytes in 2 files. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [-]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in aw-sync- rust [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on debuginfo package(s). Note: There are rpmlint messages (see attachment). [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: aw-server-rust-0.13.1^20241216.gita0cdef9-1.fc42.x86_64.rpm aw-sync-rust-0.13.1^20241216.gita0cdef9-1.fc42.x86_64.rpm aw-server-rust-0.13.1^20241216.gita0cdef9-1.fc42.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpbzk62ypx')] checks: 32, packages: 3
3 packages and 0 specfiles checked; 0 errors, 0 warnings, 11 filtered, 0 badness; has taken 0.4 s
Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output:
Rpmlint (installed packages) ---------------------------- /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory ============================ rpmlint session starts ============================ rpmlint: 2.6.1 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 3
3 packages and 0 specfiles checked; 0 errors, 0 warnings, 18 filtered, 0 badness; has taken 0.4 s
Source checksums ---------------- https://github.com/ActivityWatch/aw-server-rust/archive/a0cdef90cf86cd8d2cc8... : CHECKSUM(SHA256) this package : 1871077fdc0a8a0ff4fe2ce0d3c934ac86ab9c1aade3bce7b93ef51665ce8601 CHECKSUM(SHA256) upstream package : 1871077fdc0a8a0ff4fe2ce0d3c934ac86ab9c1aade3bce7b93ef51665ce8601
Requires -------- aw-server-rust (rpmlib, GLIBC filtered): /bin/sh ld-linux-x86-64.so.2()(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3)(64bit) libgcc_s.so.1(GCC_4.2.0)(64bit) libm.so.6()(64bit) libsqlite3.so.0()(64bit) rtld(GNU_HASH)
aw-sync-rust (rpmlib, GLIBC filtered): aw-server-rust ld-linux-x86-64.so.2()(64bit) libc.so.6()(64bit) libcrypto.so.3()(64bit) libcrypto.so.3(OPENSSL_3.0.0)(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3)(64bit) libgcc_s.so.1(GCC_4.2.0)(64bit) libm.so.6()(64bit) libsqlite3.so.0()(64bit) libssl.so.3()(64bit) libssl.so.3(OPENSSL_3.0.0)(64bit) rtld(GNU_HASH)
Provides -------- aw-server-rust: aw-server-rust aw-server-rust(x86-64)
aw-sync-rust: aw-sync-rust aw-sync-rust(x86-64)
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |RELEASE_PENDING
--- Comment #28 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/aw-server-rust
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
--- Comment #29 from wojnilowicz lukasz.wojnilowicz@gmail.com --- (In reply to Fabio Valentini from comment #27)
(In reply to wojnilowicz from comment #24)
Thanks. I feel, I would not get that easy to that.
True 😓
It would be good to have a tool to do this automatically. I filed an RFE for it: https://pagure.io/fedora-rust/rust2rpm/issue/306
That's a good idea. I have aw-awatcher package (similar to this) in queue.
Thanks for reviewing this one.
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RELEASE_PENDING |MODIFIED
--- Comment #30 from Fedora Update System updates@fedoraproject.org --- FEDORA-2025-34e640fe8b (aw-server-rust-0.13.1^20241216.gita0cdef9-1.fc42) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2025-34e640fe8b
https://bugzilla.redhat.com/show_bug.cgi?id=2327650
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |CLOSED Resolution|--- |ERRATA Last Closed| |2025-01-27 17:53:21
--- Comment #31 from Fedora Update System updates@fedoraproject.org --- FEDORA-2025-34e640fe8b (aw-server-rust-0.13.1^20241216.gita0cdef9-1.fc42) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com