Skip to content

chore: Refactor test.sh #165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 27, 2025
Merged

chore: Refactor test.sh #165

merged 1 commit into from
May 27, 2025

Conversation

polarathene
Copy link
Contributor

I intended this to be a bit smaller but figured I might as well tidy up the whole file while I'm at it.


Despite the large diff, it should function effectively the same, except docker_build now uses ldd in the container instead of file, like the other tests as the QEMU bug has been resolved since Aug 2023 (QEMU 8.1.0).

I normalized some of the script for consistency:

  • The other two "helper" methods seemed dated and only intended for troubleshooting reference during development? I split out the common verification portion to a separate function call check_crate_build_locally().
  • The read-only state -r assigned to the crate var (now CRATE_NAME) seemed unnecessary. The variable was hoisted out of the functions, thus they can't be prefixed local (given it's a small script this seems fine).
  • The original intent of the PR swaps file for ldd, and I adjusted the command to a multi-line string input, similar to HereDoc usage in Dockerfile it is like an inline shell script 👍 (easier to grok).
    • Set --workdir so no need to cd in the command given.
    • The helper equivalent that checks a fixed x86_64 target locally is now formatted the same way but differs in path checked so I added a CRATE_ARTIFACT variable 😅

When multi-arch / ARM64 image support was added to the project, there was this PR comment added about an ldd bug when relying on QEMU:

muslrust/test.sh

Lines 20 to 29 in 6b436d1

# Ideally we would use `ldd` but due to a qemu bug we can't :(
# See https://github.com/multiarch/qemu-user-static/issues/172
# Instead we use `file`.
docker run --rm \
-v "$PWD:/volume" \
-e RUST_BACKTRACE=1 \
--platform "${PLATFORM}" \
test-runner \
bash -c "cd volume; ./target/$TARGET_DIR/debug/${crate} && file ./target/$TARGET_DIR/debug/${crate} && file /volume/target/$TARGET_DIR/debug/${crate} 2>&1 | grep -qE 'static-pie linked|statically linked' && echo ${crate} is a static executable"
}

Doesn't appear to be a problem since QEMU 8.1.0 (released Aug 2023)?: multiarch/qemu-user-static#172 (comment)

Effectively the same, except `docker_build` now uses `ldd` in the container instead of `file`, like the other tests as the QEMU bug has been resolved since Aug 2023 (QEMU 8.1.0).
Copy link
Contributor Author

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Worth noting, this PR makes the need for Dockerfile.test-runner redundant.

You could just run ubuntu:noble and use ldd from there. Assuming the rustmusl-temp built image is not used to verify in a different environment.

@clux
Copy link
Owner

clux commented May 24, 2025

Assuming the rustmusl-temp built image is not used to verify in a different environment.

😅 i am (occasionally) testing arm on a mac, getting some issues there;

$ just test
just _t_macos_aarch64 plain
+ CRATE_NAME=plaincrate
+ CRATE_PATH=./test/plaincrate
+ docker_build
+ echo 'Target dir: aarch64-unknown-linux-musl'
Target dir: aarch64-unknown-linux-musl
+ echo 'Platform: linux/arm64'
Platform: linux/arm64
+ docker run --rm -it --env RUST_BACKTRACE=1 --volume ./test/plaincrate:/volume --volume cargo-cache:/opt/cargo/registry --platform linux/arm64 rustmusl-temp cargo build

compile log

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.08s
+ local CRATE_ARTIFACT=./target/aarch64-unknown-linux-musl/debug/plaincrate
+ docker run --rm -it --env RUST_BACKTRACE=1 --volume ./test/plaincrate:/volume --workdir /volume --platform linux/arm64 test-runner bash -ex -o pipefail -c '
      '\''./target/aarch64-unknown-linux-musl/debug/plaincrate'\''
      ldd '\''./target/aarch64-unknown-linux-musl/debug/plaincrate'\'' 2>&1         | grep -qE '\''not a dynamic|statically linked'\''         && echo '\'' is a static executable'\''
    '
+ ./target/aarch64-unknown-linux-musl/debug/plaincrate
Hello, visitor number 3469763539
+ ldd ./target/aarch64-unknown-linux-musl/debug/plaincrate
+ grep -qE 'not a dynamic|statically linked'
error: Recipe `_t_macos_aarch64` failed with exit code 1
error: Recipe `_ti` failed on line 32 with exit code 1

EDIT: dumping ldd output (by editing test.sh) i still get this though 🤔

+ ldd ./target/aarch64-unknown-linux-musl/debug/plaincrate
        not a dynamic executable

EDIT2; it's the pipefail option. updated and referenced in comment.

@polarathene
Copy link
Contributor Author

polarathene commented May 24, 2025

EDIT2; it's the pipefail option. updated and referenced in comment.

UPDATE: Ah, great :)

I guess that's one of those features that don't work well with the standard bash provided by macOS. It should work fine in the container usage though:

$ docker run --rm -it localhost/example-openssl bash -ex -o pipefail -c "
  target/x86_64-unknown-linux-musl/release/example
  ldd target/x86_64-unknown-linux-musl/release/example 2>&1 \
  | grep -qE 'not a dynamic|statically linked' \
  && echo 'is a static executable'
"
+ target/x86_64-unknown-linux-musl/release/example
OpenSSL version is: OpenSSL 3.5.0 8 Apr 2025
+ ldd target/x86_64-unknown-linux-musl/release/example
+ grep -qE 'not a dynamic|statically linked'

😅 i am (occasionally) testing arm on a mac, getting some issues there;

I don't have access to a mac, but presumably either:

  • Your shell isn't compatible because it's too old (_I've seen some macOS users have issues running bash scripts in the past due to minor differences in shell support or equivalent commands on mac not being at parity.
  • If the shell script is working fine, and you're using Docker Desktop perhaps it's outdated or the equivalent QEMU support is lacking?

ldd itself is just a shell script usually to an interpreter, sometimes with some extra logic. Here's what this looks like for me if I skip ldd and use it directly:

$ docker run --rm --platform "linux/arm64" ubuntu:noble /lib/ld-linux-aarch64.so.1 --list /usr/bin/bash
         (0x0000400000810000)
        libtinfo.so.6 => /lib/aarch64-linux-gnu/libtinfo.so.6 (0x00004000009b0000)
        libc.so.6 => /lib/aarch64-linux-gnu/libc.so.6 (0x0000400000a00000)
        /lib/ld-linux-aarch64.so.1 (0x00007f9351ac9000)

# AMD64 has a different filename and location:
$ docker run --rm   --platform "linux/amd64" ubuntu:noble /lib64/ld-linux-x86-64.so.2 --list /usr/bin/bash
        linux-vdso.so.1 (0x00007ffe99359000)
        libtinfo.so.6 => /lib/x86_64-linux-gnu/libtinfo.so.6 (0x00007ff57385a000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ff573648000)
        /lib64/ld-linux-x86-64.so.2 (0x00007ff5739fe000)

# Alternatively:
$ docker run --rm   --platform "linux/amd64" ubuntu:noble /lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 --list /usr/bin/bash
        linux-vdso.so.1 (0x00007fff81d16000)
        libtinfo.so.6 => /lib/x86_64-linux-gnu/libtinfo.so.6 (0x00007ff8c1d03000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ff8c1af1000)
        /lib64/ld-linux-x86-64.so.2 => /lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 (0x00007ff8c1ea7000)

If you can't run that successfully on macOS and you have the latest Docker Desktop, then I'm not sure what else you can do 😅

patchelf is another option (but like file it is a separate package you need to install):

$ patchelf --print-interpreter target/x86_64-unknown-linux-musl/release/example
patchelf: cannot find section '.interp'. The input file is most likely statically linked

$ patchelf --print-interpreter /usr/bin/bash
/lib64/ld-linux-x86-64.so.2

@clux
Copy link
Owner

clux commented May 27, 2025

Thanks for the investigation, tried a little more. I am unfortunately pretty up to date with docker desktop and bash. I wouldn't have expected bash to matter in the container (but old bash is definitely a thing elsewhere yeah). It is possible that the test container behaves differently on mac and has different features of packages compiled there, but can't think of where to go investigating this.

At any rate, I'm just going to merge and remove the pipefail option because the test runner script errors without it on non-static binaries (on both platforms). Think your 'unlikely to be relevant in practice' comment is probably true.

@clux clux merged commit 1978758 into clux:main May 27, 2025
4 of 6 checks passed
clux added a commit that referenced this pull request May 27, 2025
clux added a commit that referenced this pull request May 27, 2025
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