https://bugzilla.redhat.com/show_bug.cgi?id=2181039 --- Comment #3 from fedora.dm0@xxxxxxxxx --- The spec and SRPM are updated. (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. 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. > 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. > 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. > 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. > 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). -- 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=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
