https://bugzilla.redhat.com/show_bug.cgi?id=2238751 --- Comment #11 from Marc-Andre Lureau <marcandre.lureau@xxxxxxxxxx> --- (In reply to Fabio Valentini from comment #8) > Thanks for the update, and sorry for the wait. No worries (the QEMU side isn't yet merged upstream, and I am not sure when we will actually have users) > 1. The "make-snapshot.sh" script is not included in the SRPM. It should be > included as a SOURCE, otherwise it's impossible to re-create the srpm from > its contents. fixed > > 2. The %cargo_license macro has an "-a" flag (for "--all-features"), which > will make its results include things that aren't actually linked into the > binary. Please drop the "-a" flag here. ok > > 3. The crates do not set their license in crate metadata, which trips up the > %cargo_license* macros. It would be great if upstream set `package.license = > "BSD-3-Clause"` for both crates ... but until that's the case, it might be a > good idea to patch that in manually so the macros produce correct / complete > output. It seems they considered it: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4082335 I don't see what difference there is between the Chromium LICENSE and BSD-3 though. > > 4. The licenses of all statically linked dependencies are listed in > LICENSE.dependencies, but the License tag itself does not reflect them. > Looking at the contents of the LICENSE.dependencies file from the built > package, the License tag in the spec should look something like this: > > """ > # rutabaga_gfx: BSD-3-Clause > # crate dependencies: > # - BSD-2-Clause > # - MIT OR Apache-2.0 > # - MIT > # - Unlicense OR MIT > License: BSD-3-Clause AND BSD-2-Clause AND MIT AND (MIT OR > Apache-2.0) AND (Unlicense OR MIT) > """ fixed, thanks > > 5. The patches don't have explanations or justifications, please add a short > comment for each one, like > > """ > # drop a Windows-specific dependency > Patch0000: drop-winapi.patch > # fix filename for installed library and set up symlinks correctly > Patch0001: fix-libs.patch > # set library SONAME correctly > Patch0002: set-soname.patch > """ done > > (I'm assuming that's the reason for the patches, but I'm just guessing, > since they're not documented yet :)) > From what it looks like, the "fix-libs.patch" could be dropped entirely, > since "make" is no longer used to install the files? done > > 7. You could drop the BuildRequires for "make" since it is no longer used. ok > > 8. There's no URL to the upstream project in the package metadata. > > I would add this instead of only having the url in a comment: > URL: https://chromium.googlesource.com/crosvm/crosvm ok > > 9. The placement of the "%generate_buildrequires" scriptlet (between RPM > tags and %description) is very unusual. Please place it between %prep and > %build (since that's the order in which the scriptlets are executed). Hmm, I thought it was there initially and you asked me to move it. I realize now you just asked for an extra empty line, ok.. (confusing) > > 10. Please bump the BuildRequires from "rust-packaging >= 21" to > "cargo-rpm-macros". > The "rust-packaging" package no longer exists and is only provided by > "cargo-rpm-macros" for backwards compatibility. ok -- 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=2238751 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202238751%23c11 _______________________________________________ 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
