https://bugzilla.redhat.com/show_bug.cgi?id=2260849 --- Comment #6 from Fabio Valentini <decathorpe@xxxxxxxxx> --- Ok, first-pass review below. 1. You should replace the BuildRequires on "rust-packaging >= 21" with "cargo-rpm-macros". The former is ancient and no longer available, and only works for backwards compatibility. 2. If some BuildRequires are only required for running tests, then it is good form to also make them conditional: ``` %if %{with check} BuildRequires: procps-ng BuildRequires: git-core BuildRequires: git-email %endif ``` 3. It is a bit hard to tell whether the build uses the correct flags because the Makefile invokes cargo with "--quiet". Would it be possible to drop that flag? 4. You're generating the license info / summary, but not using it in the spec file. The License tag is incorrect. You could do something like this: ``` SourceLicense: GPL-2.0-only # paste license info from %cargo_license_summary here License: <construct complete license tag> ``` 5. As it is currently set up, the package creates unowned directories: ``` %{_datadir}/emacs/site-lisp/stgit.el %{_datadir}/vim/vimfiles/ftdetect/stg.vim %{_datadir}/vim/vimfiles/syntax/stg*.vim ``` This results in the following directories without owners: ``` %{_datadir}/emacs/ %{_datadir}/emacs/site-lisp/ %{_datadir}/vim/ %{_datadir}/vim/vimfiles/ %{_datadir}/vim/vimfiles/ftdetect/ %{_datadir}/vim/vimfiles/syntax/ ``` You need to either depend on emacs-filesystem (which provides the first two directories) and vim-data / vim-filesystem (which provide the last four), or explicitly also own (co-own) these directories. 6. It looks like the Makefile target for running tests only runs the stgit tests, but not the actual cargo tests? Maybe adding `%cargo_test` to the %check script would be a good idea to also run the unit tests? 7. There's a few warnings in the build.log that look like they might indicate a bug in the stgit code. Can you clarify with upstream whether this is actually the intended behaviour? ``` warning: use of deprecated method `indexmap::IndexSet::<T, S>::remove`: `remove` disrupts the set order -- use `swap_remove` or `shift_remove` for explicit behavior. --> src/stack/state.rs:256:28 | 256 | parent_set.remove(&prev_state.patches[patchname].commit.id); | ^^^^^^ | = note: `#[warn(deprecated)]` on by default warning: use of deprecated method `indexmap::IndexSet::<T, S>::take`: `take` disrupts the set order -- use `swap_take` or `shift_take` for explicit behavior. --> src/stack/transaction/mod.rs:634:17 | 634 | old.take(pn) | ^^^^ warning: `stgit` (bin "stg") generated 2 warnings ``` -- 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=2260849 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202260849%23c6 -- _______________________________________________ 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
