https://bugzilla.redhat.com/show_bug.cgi?id=2327650 --- Comment #13 from wojnilowicz <lukasz.wojnilowicz@xxxxxxxxx> --- (In reply to Fabio Valentini from comment #12) > 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. Done. > 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?) Done. > 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. > 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". 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. > 5. 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. > 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} Done. > 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. > 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. > 9. In general, I would recommend to split patches into logical units, not > split by which file they touch. Done. > 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/" Spec URL: https://wojnilowicz.fedorapeople.org/aw-server-rust.spec SRPM URL: https://wojnilowicz.fedorapeople.org/aw-server-rust-0.13.1^20241216.a0cdef9-1.fc41.src.rpm -- 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=2327650 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202327650%23c13 -- _______________________________________________ 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
