Spec URL: https://eclipseo.fedorapeople.org/for-review/rust-cradle.spec SRPM URL: https://eclipseo.fedorapeople.org/for-review/rust-cradle-0.2.2-1.fc36.src.rpm Description: Execute child processes with ease. Fedora Account System Username: eclipseo
Small problems: 1) Your Cargo.toml patch: [target."cfg(unix)".dependencies.nix] -version = "0.22.2" +version = "0.24.1" optional = true This is most probably not safe and broken, and you just don't see it because that code is optional not compiled by default. There have been quite extensive API changes between nix versions 0.23 and 0.24. Just drop this from the patch, we also have rust-nix0.22 packaged as a compat package for nix 0.22.x. [target."cfg(unix)".dependencies.gag] -version = "0.1.10" +version = "1" optional = true Same applies here. Does the code actually compile against the new version (i.e. if you call all the %cargo_* macros with "-a")? 2) Problems with how the %cargo_* macros are used: echo 'vim-common' echo 'crate(gag)' echo 'crate(nix)' echo 'crate(memoffset)' This should never be needed. If the tests require some optional dependencies to be present, use the "-a" ("--all-features") flag for %cargo_generate_buildrequires, %cargo_build, %cargo_install, and %cargo_test. 3) Workaround for test failures: > https://github.com/soenkehahn/cradle/blob/main/memory-tests/Cargo.toml This indicates that the "memory-tests" sub-crate is not published, and it's obviously also not included in published sources for cradle. Just skip the test that tries to access it (i.e. use "%cargo_test -- -- --skip memory_test") or patch it out of tests/integration.rs (it's at the bottom of the file). > "won't work in rust test cases" Just add a link to the upstream README and use --nocapture, as indicated there. In total, you should probably call %cargo_test like this: # * work around IO redirection in test harnesses with --nocapture # * skip a test that tries to read files that are not present in published crates %cargo_test -a -- -- --nocapture --skip memory_test With that, you should not need to ignore %cargo_test with || :. === If you have time, it would be great to get https://bugzilla.redhat.com/show_bug.cgi?id=2090196 reviewed, in turn. This ticket is currently blocking pending updates for the openssl / openssl-sys crates.
(In reply to Fabio Valentini from comment #1) > Small problems: > > 1) Your Cargo.toml patch: > > [target."cfg(unix)".dependencies.nix] > -version = "0.22.2" > +version = "0.24.1" > optional = true > > This is most probably not safe and broken, and you just don't see it because > that code is optional not compiled by default. > There have been quite extensive API changes between nix versions 0.23 and > 0.24. > Just drop this from the patch, we also have rust-nix0.22 packaged as a > compat package for nix 0.22.x. > But if it bilds with -a, does that mean it can stay bumped? Anyway unbumped it. > [target."cfg(unix)".dependencies.gag] > -version = "0.1.10" > +version = "1" > optional = true > > Same applies here. Does the code actually compile against the new version > (i.e. if you call all the %cargo_* macros with "-a")? The only changes between 0.1.10 and 1 were Windows fixes. No api changes. > > 2) Problems with how the %cargo_* macros are used: > > echo 'vim-common' > echo 'crate(gag)' > echo 'crate(nix)' > echo 'crate(memoffset)' > > This should never be needed. If the tests require some optional dependencies > to be present, use the "-a" ("--all-features") flag for > %cargo_generate_buildrequires, %cargo_build, %cargo_install, and %cargo_test. > Ok, done. > 3) Workaround for test failures: > > > https://github.com/soenkehahn/cradle/blob/main/memory-tests/Cargo.toml > > This indicates that the "memory-tests" sub-crate is not published, and it's > obviously also not included in published sources for cradle. > Just skip the test that tries to access it (i.e. use "%cargo_test -- -- > --skip memory_test") or patch it out of tests/integration.rs (it's at the > bottom of the file). > > > "won't work in rust test cases" > > Just add a link to the upstream README and use --nocapture, as indicated > there. > > In total, you should probably call %cargo_test like this: > > # * work around IO redirection in test harnesses with --nocapture > # * skip a test that tries to read files that are not present in published > crates > %cargo_test -a -- -- --nocapture --skip memory_test > > With that, you should not need to ignore %cargo_test with || :. > It works, thanks! > === > > If you have time, it would be great to get > https://bugzilla.redhat.com/show_bug.cgi?id=2090196 reviewed, in turn. > This ticket is currently blocking pending updates for the openssl / > openssl-sys crates. Will do
New Spec URL: https://eclipseo.fedorapeople.org/for-review/rust-cradle.spec New SRPM URL: https://eclipseo.fedorapeople.org/for-review/rust-cradle-0.2.2-1.fc36.src.rpm
> But if it bilds with -a, does that mean it can stay bumped? Anyway unbumped it. I think it can. According to the release notes, the most invasive change for nix 0.24 was that features can now be enabled separately. But by default, everything is still enabled. So since this crates uses default features, it should not be affected by this change. https://github.com/nix-rust/nix/blob/master/CHANGELOG.md#0240---2022-04-21 === I now see that you're also manually removing test binaries and the binary subpackage from the .spec file. You'll also need to define __cargo_is_bin false to fully disable the "build a binary" mode, like here: https://src.fedoraproject.org/rpms/rust-assert_cmd/blob/rawhide/f/rust-assert_cmd.spec#_5 That way, the binaries won't be installed, and you won't need to remove them manually (drop the lines below entirely): > # We don't actually need this > rm -rfv %{buildroot}%{_bindir}/context_integration_tests \ > %{buildroot}%{_bindir}/test_executables_helper \ > %{buildroot}%{_bindir}/test_executables_panic === Please also make sure to remove all unwanted files from the shipped package. Either by adding the following to the Cargo.toml: exclude = ["/justfile"] Or by adding this line to "%files devel" in the .spec file: %exclude %{cargo_instdir}/justfile Not sure if the README.php file can cause issues, but at least it doesn't generate an automatic dependency on /usr/bin/php ...
I have imported rust-gag for you. Please proceed with this review request, if possible. Otherwise somebody else might want to continue working on fixing rust-just (which has now been broken for so long that it's been orphaned and will be retired in 4-5 weeks).
rust-just has now been broken for so long that it has been retired from F37+. If you still want to fix it and update it to the latest version, you'll need to file an un-retirement request ticket with releng.
"just" has been unretired and updated, and the dependency on "cradle" was patched out. Additionally, CC0 is no longer an acceptable license for code, so I suggest this review request is dropped.
Do you still need this package? If not, please close the review request.
This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it.