https://bugzilla.redhat.com/show_bug.cgi?id=2428704
--- Comment #19 from Fabio Valentini <[email protected]> --- Another (hopefully, full) review of the latest submission included below. Other issues appear to have been resolved, only items that should or must still be addressed are still mentioned. Minor issues (non-blocking) =========================== You define a "stable_version" macro and use it exactly once (to set Version). You could just omit that useless indirection and keep the comment above it. Major issues (blocking) ======================= a. You reference a "generate-vendor-tarball.sh" script that is used to generate the tarball with vendored sources. But this script is not included - you should include this as a "Source" file so it is included in `.src.rpm` files. On a positive note: The script linked in the comment above looks OK and appears to work correctly. I was able to reproduce the goose-%{version}-vendor.tar.xz file with it, and the contents look identical - but the checksum of the tarball is different from the one you provided. I think this is expected due to how "tar" works with default options, so that should be fine. b. The license files for included JavaScript libraries are now included (good!) but the Cargo.toml change is wrong (and unnecessary): https://github.com/block/goose/commit/37f419d I'm not sure why you thought modifying the "package.include" setting in Cargo.toml was needed (it's not). As such, it likely won't have any ill effects (since the goose subcomponents are not published on crates.io), but overriding the "package.include" setting like this means "don't include *any other files except these*". I'm not sure how this PR was reviewed and accepted as-is: https://github.com/block/goose/pull/7352 Even the "Testing" section you included in the PR description shows that the published crate would end up without any contents *except* Cargo.toml, README.md, and the license texts, i.e. no source code at all. Also I wonder why you thought "cargo package --list" would be a good "testing" indicator for a subproject that is not published on crates.io. c. The License tag is ... better, but still wrong / incomplete / contains items that are unexplained: 1. I can't see where the "(Apache-2.0 OR BSL-1.0 OR MIT)" term comes from, it's not in the output of %cargo_license_summary and doesn't match any non-Rust component either. 2. The same applies to "(Apache-2.0 OR ISC OR MIT)" which is unexplained. 3. The error from fedora-review above is also correct - running "license-verify", the License tag is indeed invalid. The error points at "GPL-2.0" which is not considered a valid identifier by Fedora (and it is deprecated in recent SPDX license list versions). I'm also not sure where it comes from - the only occurrence of a GPL-2.0 license is in "Apache-2.0 OR GPL-2.0-only" from the "%cargo_license_summary" output, where the identifier is the *correct* one. d. The given instructions for generating the list of bundled Sublime Text grammars and themes are broken. I'm not sure what these were meant to be / supposed to do: "cd %%{crate}-%%{version}/vendor/syntect-*/assets/default_newlines.packdump | xxd" "cd %%{crate}-%%{version}/vendor/syntect-*/assets/default.themedump | xxd" The "cd" command doesn't print output that can be piped anywhere, and the "%crate" macro is not defined. I tried running "cat default.themedump | xxd" thinking that maybe *that's* what you meant to write, but this doesn't produce any usable output (other than ... well, a hexdump of what looks like random / compressed data). -- 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=2428704 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202428704%23c19 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://forge.fedoraproject.org/infra/tickets/issues/new
