Skip to content

Conversation

Ecordonnier
Copy link
Contributor

@Ecordonnier Ecordonnier commented May 22, 2025

Commit 2:

Summary:

Partial fix for #6591

The current code declare libstdbuf as a build-dependency of stdbuf as a
workaround to enforce that libstdbuf is compiled before stdbuf. This breaks
cross-compilation, because build-dependencies were compiled for the host
architecture, and not for the target architecture.

The reason this workaround is necessary is that bindeps is available only in nightly at the moment:
https://rust-lang.github.io/rfcs/3028-cargo-binary-dependencies.html

This commit replaces the "build-dependency" workaround with another workaround:
calling cargo manually to build libstdbuf in the build.rs of stdbuf, in order to ensure that libstdbuf is built before stdbuf.

Changes:

  • Removed cpp/cpp_build dependencies:

The cpp, cpp_build, and related dependencies were removed because they made cross-compilation in a build.rs file >very complex, since you need
to pass proper CXX env variables for cross-compilation, whereas cross-compiling rust code using cargo is quite simple.
Provided Rust implementations for getting stdin, stdout, and stderr pointers.
Switched from C++/cpp macro-based initialization to using the Rust ctor crate for library initialization.

  • Remove "feat_require_crate_cpp" which is not needed any more, since stdbuf was the only utility using the cpp crate.

Tests:

This commit fixes this test:
cross test --target aarch64-unknown-linux-gnu --features stdbuf test_stdbuf::test_libstdbuf_preload -- --nocapture

The "i686" build of stdbuf was also broken (stdbuf 32 bits, but libstdbuf 64 bits) and test_stdbuf::test_libstdbuf_preload >of the i686 builds in github CI serves as regression
test for this issue, no need to add a cross-rs test for aarch64.

Commit 1:

stdbuf: add test_libstdbuf_preload

This test verifies that stdbuf correctly preloads libstdbuf and that there is no architecture mismatch error. At the moment the test passes when compiled natively, but fails when compiled with cross-rs, due to #6591

This passes:

cargo test --features stdbuf test_stdbuf::test_setvbuf_resolution -- --nocapture

This fails:

cross test --target aarch64-unknown-linux-gnu --features stdbuf test_stdbuf::test_libstdbuf_preoad -- --nocapture

@Ecordonnier Ecordonnier marked this pull request as draft May 22, 2025 20:29
@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 2 times, most recently from 22ae62b to b73f9be Compare May 22, 2025 21:16
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from b73f9be to bea271b Compare May 23, 2025 20:09
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 3 times, most recently from 5c306bb to da00ce9 Compare May 23, 2025 21:22
@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 3 times, most recently from 70112e5 to 58228b7 Compare May 23, 2025 21:48
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from 58228b7 to 21d8570 Compare May 24, 2025 19:38
@Ecordonnier Ecordonnier changed the title WIP stdbuf: add test_setvbuf_resolution WIP stdbuf: fix cross-compilation May 24, 2025
@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from 21d8570 to 7861de9 Compare May 24, 2025 19:59
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 4 times, most recently from 34c6d3f to 379478b Compare May 24, 2025 22:57
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier changed the title WIP stdbuf: fix cross-compilation stdbuf: fix cross-compilation May 25, 2025
@Ecordonnier Ecordonnier marked this pull request as ready for review May 25, 2025 05:48
@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 4 times, most recently from 254a667 to bc9c169 Compare May 25, 2025 06:18
@Ecordonnier Ecordonnier marked this pull request as draft May 25, 2025 06:28
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 4 times, most recently from bf115e3 to de124c7 Compare May 25, 2025 09:38
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from de124c7 to 82f0bda Compare May 25, 2025 19:31
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 2 times, most recently from 73b509f to 5fa7727 Compare May 25, 2025 20:59
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)

@Ecordonnier Ecordonnier marked this pull request as ready for review May 25, 2025 21:57
@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from 5fa7727 to b091393 Compare May 26, 2025 09:22
@Ecordonnier
Copy link
Contributor Author

Ecordonnier commented May 26, 2025

@sylvestre the branch should be ready for review. There is some intermittent issue with the CI jobs not related to the branch, but everything except cargo-deny was green before my latest rebase. My last commit fixes the cargo-deny error.

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch 2 times, most recently from ff73bdf to 73c2fb0 Compare May 28, 2025 09:04
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from 73c2fb0 to cf83d01 Compare May 29, 2025 09:54
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

// for include_bytes!(..."/libstdbuf.so") to work.
// In the future, "bindeps" should be used to simplify the code and avoid the manual cargo call,
// however this is available only in cargo nightly at the moment.
let mut cmd = Command::new(&cargo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i really don't like Command calls, they are to debug, can break dependencies analysis, etc :(
about bindeps, could you please add a link to the issue? thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is ugly, but I haven't found a more elegant solution :-/
I was surprised that cargo does not support something like bindeps yet. It seems quite a basic requirement to be able to say "build A before B".
I added a link to rust-lang/cargo#9096 in the comment.

At least the good thing is that once bindeps is stabilized, the Command call can be removed, and something like this added to the Cargo.toml of stdbuf:

[dependencies]
libstdbuf = { ..., artifact = "cdylib" }

@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from cf83d01 to bf8abac Compare May 30, 2025 10:33
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)

This test verifies that libstdbuf correctly gets preloaded, and that there are no architecture mismatch errors.
At the moment the test passes when compiled normally, but fails when compiled with cross-rs, due to uutils#6591

This passes:
```
cargo test --features stdbuf test_stdbuf::test_libstdbuf_preload -- --nocapture
```

This fails:
```
cross test --target aarch64-unknown-linux-gnu --features stdbuf test_stdbuf::test_libstdbuf_preload -- --nocapture
```

Signed-off-by: Etienne Cordonnier <[email protected]>
Summary:

Partial fix for uutils#6591

The current code declare libstdbuf as a build-dependency of stdbuf as a
workaround to enforce that libstdbuf is compiled before stdbuf. This breaks
cross-compilation, because build-dependencies were compiled for the host
architecture, and not for the target architecture.

The reason this workaround is necessary is that bindeps is available only in nightly at the moment:
https://rust-lang.github.io/rfcs/3028-cargo-binary-dependencies.html

This commit replaces the "build-dependency" workaround with another workaround:
calling cargo manually to build libstdbuf in the build.rs of stdbuf, in order to ensure that libstdbuf is built before stdbuf.

Changes:

- Removed cpp/cpp_build dependencies:

The cpp, cpp_build, and related dependencies were removed because they made cross-compilation in a build.rs file very complex, since you need
to pass proper CXX env variables for cross-compilation, whereas cross-compiling rust code using cargo is quite simple.
Provided Rust implementations for getting stdin, stdout, and stderr pointers.
Switched from C++/cpp macro-based initialization to using the Rust ctor crate for library initialization.

- Remove "feat_require_crate_cpp" which is not needed any more, since stdbuf was the only utility using the cpp crate.

Tests:

This commit fixes e.g. this test:
cross test --target aarch64-unknown-linux-gnu --features stdbuf test_stdbuf::test_libstdbuf_preload -- --nocapture

- The "i686" build of stdbuf was also broken (stdbuf 32 bits, but libstdbuf 64 bits) and test_stdbuf::test_libstdbuf_preload of the i686 builds in github CI serves as regression
test for this issue, no need to add a cross-rs test for aarch64.
- The x86_64 musl build of stdbuf was also broken and was passing tests in CI only because it was compiled with the wrong libc (glibc instead of musl)

Signed-off-by: Etienne Cordonnier <[email protected]>
@Ecordonnier Ecordonnier force-pushed the eco/stdbuf-regression-test branch from bf8abac to 35634b4 Compare June 2, 2025 08:57
Copy link

github-actions bot commented Jun 2, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@sylvestre
Copy link
Contributor

I am not very happy with the command but I don't have a better solution to propose. Let's see if it sticks.

@sylvestre sylvestre merged commit b5d4b0e into uutils:main Jun 2, 2025
74 checks passed
@Ecordonnier Ecordonnier deleted the eco/stdbuf-regression-test branch June 2, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants