https://bugzilla.redhat.com/show_bug.cgi?id=2181039 --- Comment #4 from Fabio Valentini <decathorpe@xxxxxxxxx> --- (In reply to fedora.dm0 from comment #3) > The spec and SRPM are updated. Thanks! I left a few more comments. > (In reply to Fabio Valentini from comment #2) > > 1. I don't really understand what this comment is meant to say: > > > > """ # This should be left enabled for generating buildreqs correctly. > > """ %bcond_without check > > > > Yes, "check" needs to be enabled for "dev-dependencies" to be included in > > generated BuildRequires, if that's what you meant to say ... but I'm not > > sure if that needs to be said explicitly, since this is normal for Rust > > packages. > > The workspace packages share the same build.rs which seems to fail if their > dev-dependencies aren't installed. That seems strange and sounds like a bug somewhere :( > %{__cargo_to_rpm} --path Cargo.toml buildrequires --with-check Macros that include double underscores are implementation details and shouldn't be used directly. If you really want to override this behaviour to work around the bug, just use "cargo2rpm" instead of "%__cargo_to_rpm". > https://github.com/firecracker-microvm/firecracker/blob/v1.3.1/src/ > firecracker/Cargo.toml#L6 > > I've just changed %generate_buildrequires to always pass --with-check and > dropped the comment. > > > 2. Building for the *-musl Rust targets is not going to be supported in > > Fedora (we don't even have the musl toolchain targets available). > > All conditionals that rely on %{cargo_target} and / or %{with jailer} are > > noops, please remove them. > > Will I have no discretion as maintainer to provide build options that do not > conflict with the guidelines? As was mentioned in the original e-mail > thread, Firecracker's security benefits (seccomp, sandboxing) currently only > apply to the musl targets. If I can't support a non-default option to use > it, I will have to maintain a secure version of the spec separately. I > don't see a downside to allowing other developers to run "rpmbuild -D ..." > to produce the full build as needed. If you want to continue maintaining the conditionals even though they will not be used in Fedora, that is your decision. I based my recommendation on this rule: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_legibility But in this case, the additions are not *too bad*, so I'm fine with you keeping them. > > 3. The Source URLs use the old and deprecated format that relies on the URL > > "#/fragment" hack. Please use the "new" format (which has worked for almost > > a decade), i.e. > > Source0: > > https://github.com/firecracker-microvm/firecracker/archive/v%{version}/ > > %{name}-%{version}.tar.gz > > instead of: > > Source0: > > https://github.com/firecracker-microvm/firecracker/archive/refs/tags/ > > v%{version}.tar.gz#/%{name}-%{version}.tgz > > Same applies to the URLs for Source1 and Source2. > > > > Documentation for this (both for using tarballs for git tags or for specific > > commits) is here: > > https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > > #_commit_revision > > > > 4. Strange names for downloads of bundled crates. > > I'm not sure why you're downloading kvm-bindings-e835920.tar.gz and > > micro-http-4b18a04.tar.gz but store them as kvm-bindings-0.6.0-1.crate and > > micro_http-0.1.0.crate. This is confusing and unnecessary, since you control > > all parts of this process, there's no need to name them as if they were > > upstream crates. > > URLs are updated. Thanks! You could even use shortened hashes, but that's a stylistic choice - i.e. this would work fine too, and would be less verbose: Source1: https://github.com/firecracker-microvm/kvm-bindings/archive/e835920/kvm-bindings-e835920.tar.gz Source2: https://github.com/firecracker-microvm/micro-http/archive/4b18a04/micro_http-4b18a04.tar.gz > > 5. Patches are included without comments or explanations. > > Please include short comments about what your patches do to the spec file, > > and whether they are suitable for upstreaming (or not). > > Comments are added to the spec. Thanks! Looks good to me. > > 6. Don't ignore test failures. > > It's OK to ignore specific tests that don't work in our containerized build > > environment, but "|| :"-ing everything is not OK. > > I did that to follow what the rust package does. > > https://src.fedoraproject.org/rpms/rust/blob/ > 7ac7a42b5e2fd46d73d2a0946c55109c03e22966/f/rust.spec#_874-886 > > There are over 100 test failures on x86_64 alone, and the Rust test --skip > flag seems broken for doc tests. (I want to use --exact instead of > substring filtering since there are so many tests, and e.g. > --skip=filter_cpuid doesn't match the exact test name, but the full string > --skip='src/lib.rs - filter_cpuid (line 43)' causes every doc test for every > package to be skipped.) > > I've dropped the "|| :" and disabled tests by default instead since that > seems acceptable judging by similar packages, although we won't see results > in the log anymore. Makes sense to me. Running the tests but ignoring the results is usually a waste of resources IMO ... In cases like this where test results are almost entirely useless due to failures in the containerized build environment, it makes more sense to disable them by default. > > 7. Include breakdown of the dependencies / licenses. > > The License tag looks comprehensive, but there's no indication how this list > > was generated (you're not using the %cargo_license or %cargo_license_summary > > macros in the spec file), and there's no breakdown that lists which > > dependency is available under which license. > > It was generated by copying the rust-* package dependencies out of a build > log and running this (and manually removing any equivalent SPDX expressions): > > dnf repoquery --queryformat='%{LICENSE}\n' $(<deps.txt) | sort -u > > I've added a LICENSE.dependencies file from %cargo_license. I'm assuming > that's what it's supposed to do based on how a couple packages use it--I > don't see it mentioned in the guidelines, and Google has zero results for > 'fedora "cargo_license"'. > > I've also dropped ISC from the License field since that's only on libloading > (used by bindgen, not linked into the binaries). Yeah, as you have noticed, the "dnf repoquery" command you used for creating the list includes *too many things* (i.e. it also includes crates that are only used at build time, like [build-dependencies] and crates that only provide macros (like bindgen + its dependency tree). It also references things in a mix of old Fedora style license identifiers and new SPDX license identifiers, because not all Rust packages have been migrated yet. The %cargo_license(_summary) macros are supposed to be solving exactly this problem - get the tree of cargo dependencies *excluding* dev-, build-, and proc-macro-only dependencies, and print the result in SPDX format *only*. It's still pretty new and I still need to document it ... > BuildRequires: rust-packaging This is also outdated, the new package name is cargo-rpm-macros (and the %cargo_license macro is only present in rust-packaging / cargo-rpm-macros >= 23), and cargo2rpm that you use to generate BuildRequires is only present with "cargo-rpm-macros >= 24", so it should probably just be "BuildRequires: cargo-rpm-macros >= 24". Other than these few small-ish things, the package looks pretty good to me now, thanks! -- 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=2181039 _______________________________________________ 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
