https://bugzilla.redhat.com/show_bug.cgi?id=1942392
--- Comment #3 from Sergio Lopez <slopezpa(a)redhat.com> ---
(In reply to Connor Kuehl from comment #2)
Hi Sergio!
Hi, Connor. Thanks for the review!
Overall it looks good. Just a few requested changes:
1. The vmm-sys-util-fix-metadata.diff should be sent upstream, otherwise
their crates.io package is needlessly including that CI stuff
Sure, I intend to do that for vmm-sys-util and, pretty much, every other
rust-vmm crate.
1.1. This diff doesn't do what it looks like it should do. Since
upstream
doesn't exclude this in their manifest, patching it out here is too late, as
it's already included in the *.crate source tarball. I suspect you'll want
to update this patch to actually remove the rust-vmm-ci/ directory and the
/coverage_config_* files.
This is indicated in the Rust Packaging Guidelines [1] and other packages, such
as rust-memmap, include similar patches. I've tested it and doing this prevents
those files to be included in the -devel package.
[1]
https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_excludin...
2. I think the other patch, vmm-sys-util-omit-ioctl-tests.diff, is
fine, but
if you wanted to, you can automatically patch out the tests by changing
'%bcond_without check' to '%bcond_with check' in the .spec file which
would
mean that this patch could also be removed. This isn't a required change for
this review though.
But that would disable all tests, wouldn't it?
3. The %license field is missing
4. The changelog is slightly different than one of the prescribed formats
(
https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs).
I've been wondering if rust2rpm should stop putting the HH:MM:SS timestamp
in the changelog entry. It's on my list to ask the other Rust packagers +
upstream rust2rpm.
Ouch. I'll fix both right away.
--
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