Bug 2103380 (rust-cradle) - Review Request: rust-cradle - Execute child processes with ease
Summary: Review Request: rust-cradle - Execute child processes with ease
Keywords:
Status: CLOSED NOTABUG
Alias: rust-cradle
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: rust-gag
Blocks: FE-DEADREVIEW 1984770
TreeView+ depends on / blocked
 
Reported: 2022-07-02 18:51 UTC by Robert-André Mauchin 🐧
Modified: 2023-03-02 00:45 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-03-02 00:45:28 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Robert-André Mauchin 🐧 2022-07-02 18:51:01 UTC
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

Comment 1 Fabio Valentini 2022-07-03 10:43:10 UTC
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.

Comment 2 Robert-André Mauchin 🐧 2022-07-03 13:41:51 UTC
(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

Comment 4 Fabio Valentini 2022-07-03 14:22:57 UTC
> 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 ...

Comment 5 Fabio Valentini 2022-09-14 14:20:16 UTC
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).

Comment 6 Fabio Valentini 2022-09-28 16:11:13 UTC
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.

Comment 7 Fabio Valentini 2022-10-14 12:15:54 UTC
"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.

Comment 8 Fabio Valentini 2023-01-30 19:32:07 UTC
Do you still need this package? If not, please close the review request.

Comment 9 Package Review 2023-03-02 00:45:28 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.