Skip to content
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.

Sign up for GitHub

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

Jump to bottom

set QEMU_LD_PREFIX in the runners in order to be able to use env -i #1420

Merged
merged 7 commits into from
Jan 30, 2024
Merged

set QEMU_LD_PREFIX in the runners in order to be able to use env -i #1420

merged 7 commits into from
Jan 30, 2024

Conversation

glehmann
Copy link
Contributor

I'm working on making assert_cmd working with cross.

There is one case a bit more difficult:
when using clear_env() on a process, or env -i, the binary is not able to run anymore because the
QEMU_LD_PREFIX envvar is not set anymore.

A (relatively) easy fix might be to ensure in linux-runner — and in any other runner — that the
necessary environment is configured before running the executable. Something as proposed
in this PR for aarch64-unknown-linux-gnu.

Would that be an acceptable solution?

assert-rs/assert_cmd#193

@glehmann glehmann requested a review from a team as a code owner January 24, 2024 21:46
@Emilgardis Emilgardis added enhancement A-qemu Area: qemu runners labels Jan 24, 2024
Emilgardis
Emilgardis requested changes Jan 24, 2024
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

Nice workaround! I'm fine with this approach, I would however like to see it done in all images and not just aarch64!

I also have a slight concern about what happens if the env is wiped and @DEFAULT_QEMU_LD_PREFIX@ hasn't been replaced by us. We should, if this lands, ensure that https://github.com/cross-rs/cross-toolchains also has been updated

docker/linux-runner Outdated Show resolved Hide resolved
@glehmann
Copy link
Contributor Author

Nice workaround! I'm fine with this approach, I would however like to see it done in all images and not just aarch64!

Sure! I was just checking that you were ok with that before patching all the images — there is quite a lot :)

I'm thinking to start by adding a test that runs env -i ${CROSS_TARGET_RUNNER} /a/command. That way we should be sure that we have the minimum environment configured in each image.

I also have a slight concern about what happens if the env is wiped and @DEFAULT_QEMU_LD_PREFIX@ hasn't been replaced by us.

@DEFAULT_QEMU_LD_PREFIX@ is just the default value in ${QEMU_LD_PREFIX:-@DEFAULT_QEMU_LD_PREFIX@}, and QEMU_LD_PREFIX is still set in the docker image. So if @DEFAULT_QEMU_LD_PREFIX@ is not replaced by its value, cross will continue to work as it does today: just fine in most case, and not able to run the executable if the env is cleaned.

We should, if this lands, ensure that https://github.com/cross-rs/cross-toolchains also has been updated

ok, just a few more images ;-)

@Emilgardis
Copy link
Member

@DEFAULT_QEMU_LD_PREFIX@ is just the default value in ${QEMU_LD_PREFIX:-@DEFAULT_QEMU_LD_PREFIX@}, and QEMU_LD_PREFIX is still set in the docker image. So if @DEFAULT_QEMU_LD_PREFIX@ is not replaced by its value, cross will continue to work as it does today: just fine in most case, and not able to run the executable if the env is cleaned.

I understand this, my concern is simply about what qemu does when the env has been cleared, the default has not been replaced by the docker build and the only path in QEMU_LD_PREFIX is a relative-looking non-existant path (@DEFAULT_QEMU_LD_PREFIX@). I don't think anything bad will happen but just wanted to raise it :D.

rg -l 'CROSS_TARGET_RUNNER=' -g 'Dockerfile.*linux*' should be all the files that need to be fixed (haven't tested this, no access to my computer rn)

@glehmann glehmann force-pushed the qemu-ld-prefix-in-linux-runner branch 2 times, most recently from ff6c46c to 029e305 Compare January 26, 2024 07:09
@glehmann
Copy link
Contributor Author

I understand this, my concern is simply about what qemu does when the env has been cleared, the default has not been replaced by the docker build and the only path in QEMU_LD_PREFIX is a relative-looking non-existant path (@DEFAULT_QEMU_LD_PREFIX@). I don't think anything bad will happen but just wanted to raise it :D.

so I've tested it:

  • the run fails in the exact same way when the environment variables are cleared as when QEMU_LD_PREFIX is not set. Qemu is probably checking if some files exist in @DEFAULT_QEMU_LD_PREFIX@, but there's nothing visible for the end user.
  • everything runs at it should when not clearing the environment variables

@glehmann glehmann changed the title WIP: set QEMU_LD_PREFIX in linux-runner in order to be able to use env -i set QEMU_LD_PREFIX in linux-runner in order to be able to use env -i Jan 26, 2024
@glehmann glehmann changed the title set QEMU_LD_PREFIX in linux-runner in order to be able to use env -i set QEMU_LD_PREFIX in the runners in order to be able to use env -i Jan 26, 2024
Emilgardis
Emilgardis requested changes Jan 26, 2024
View reviewed changes
docker/android-runner Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member

/ci try -t linux

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@glehmann glehmann force-pushed the qemu-ld-prefix-in-linux-runner branch from 029e305 to e09367d Compare January 26, 2024 10:43
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Emilgardis
Copy link
Member

The errors are expected I think

@Emilgardis
Copy link
Member

I just realised something I shouldve thought about 😊 the tests dont ever hit this :D

@Emilgardis
Copy link
Member

I think we should a new testcase that either uses assert_cmd or simulates the env clearing.

@Emilgardis
Copy link
Member

/ci try -t aarch64-unknown-linux-* -t i686-linux

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@glehmann
Copy link
Contributor Author

I think the tests won't help until the PR made it to the main branch: they are not using the image from this PR.

https://github.com/cross-rs/cross/actions/runs/7679215720/job/20929912781?pr=1420##step:11:29

interesting to see that some tests are passing though — QEMU_LD_PREFIX doesn't seem to be necessary for all the targets

@Emilgardis
Copy link
Member

Emilgardis commented Jan 27, 2024 •
edited
Loading

They are definitely using the new images, see --tag ghcr.io/cross-rs/aarch64-unknown-linux-gnu:main and sed ...

@glehmann
Copy link
Contributor Author

oh, ok my bad: I've been confused by the name ghcr.io/cross-rs/aarch64-unknown-linux-gnu:main. I've downloaded it locally and it doesn't contain the changes of the PR.

I'm looking at it

@glehmann
Copy link
Contributor Author

I tried to rebuild the image from scratch and rerun the test locally — it still runs without problem.

I've set CROSS_DEBUG in test.sh to get a better chance to understand what is going on in the CI

@glehmann
Copy link
Contributor Author

/ci try -t aarch64-unknown-linux-* -t i686-linux

@glehmann
Copy link
Contributor Author

my attempt at triggering a new test run deesn't seem very effective

@Emilgardis could you trigger a new run for this PR?

@Emilgardis
Copy link
Member

/ci try -t aarch64-unknown-linux-* -t i686-linux

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Emilgardis
Copy link
Member

Emilgardis commented Jan 29, 2024 •
edited
Loading

Now thinking about this even more. The issue here is that build.rs is ran on the host right?
The problem isn't "cross run --target <target> can't execute a binary compiled for it's own <target> via std::process::Command" right?

If that's the case, the added test case doesn't test correctly, what it should do is compile launch without --target, and since that's probably hard to do, should be done in build script

@glehmann
Copy link
Contributor Author

The missing variables are CROSS_TARGETARCH, CROSS_TARGETVARIANT, they are only used in Dockerfile.native and Dockerfile.native.centos

this problem should be fixed :-)

For windows the problem is similar to the issue with qemu-system. We want to not respect the runner, just run it directly.

I've changed the test to only use the runner if it's defined in the env var — in fact that's exactly what I did in assert_cmd — and I rolled back my changes to not test qemu-system since they are now passing :-)

@glehmann
Copy link
Contributor Author

Now thinking about this even more. The issue here is that build.rs is ran on the host right? The problem isn't "cross run --target <target> can't execute a binary compiled for it's own <target> via std::process::Command" right?

the problem is to run everything for the target passed to cross. That may or may not require a runner depending on the virtualisation system used. I think the test does the right thing

I think we are not that far to have everything working :-)

@Emilgardis
Copy link
Member

/ci try

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Emilgardis
Copy link
Member

test should use CROSS_FLAGS to correctly use build-std

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@glehmann
Copy link
Contributor Author

test should use CROSS_FLAGS to correctly use build-std

mmm, I think they are, aren't they?

I don't see what I have changed that makes the bsd systems fail :-/

@glehmann
Copy link
Contributor Author

for windows, launch.exe, which is launched with wine is able to launch foo.exe directly, without calling wine.

The test fails if I make launch.exe use wine because it can't find the wine executable. So I guess I have to drop the CARGO_TARGET_<target>_RUNNER, so we don't try to use wine if we are already in wine

@Emilgardis
Copy link
Member

you removed the if RUN check, that should be there because some of these we can't run on at all

@glehmann
don't try to use the runner if it's not defined
f9bc6a8 
some virtual env require a runner, some others don't. The runner
env var is there to tell us if we need to use one or not
@glehmann
drop the runner env var once entering wine
2fef4f4 
in order to not use wine again when launching a new binary
@glehmann glehmann force-pushed the qemu-ld-prefix-in-linux-runner branch from 369b3a0 to 2fef4f4 Compare January 29, 2024 21:18
@glehmann
Copy link
Contributor Author

ok, I misunderstood what RUN was for — this is happening too often for a single PR!

@glehmann
Copy link
Contributor Author

should be ok for windows too. I've used env -u to remove the runner config from the env. Just let me know if you prefer a plain script :-)

@glehmann
Copy link
Contributor Author

and I need to look into what is going on with wasm.

The project is more complex than I thought (maybe I've been fooled by how easy it is to use), and this shows in the test suite. I'm curious to see what approach you chose to refactor the test suite in rust. Is it available in a branch?

@Emilgardis
Copy link
Member

/ci try

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Emilgardis
Copy link
Member

The project is more complex than I thought

Yes :D

I'm curious to see what approach you chose to refactor the test suite in rust. Is it available in a branch?

Its nothing concrete right now, basically replace bash with rust, then rewrite it to be prettier, we do have #1037 for the bigger issue of actual UI tests

@github-actions GitHub Actions
Copy link

Try run for comment

Failed Jobs

Successful Jobs

List

@Emilgardis
Copy link
Member

failures look fine, except the wasm one, should probably just exempt it from running this test.

@Emilgardis Emilgardis added the no-ci-targets PRs that do not affect any cross-compilation targets. label Jan 30, 2024
@glehmann
don't run the cleared env test for wasm32-unknown-emscripten
0beab02 
this target doesn't support running a command
Emilgardis
Emilgardis requested changes Jan 30, 2024
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor fix and this should be good!

ci/test.sh Show resolved Hide resolved
Emilgardis
Emilgardis approved these changes Jan 30, 2024
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

Thank you!

@Emilgardis Emilgardis added this pull request to the merge queue Jan 30, 2024
Merged via the queue into cross-rs:main with commit 047eb57 Jan 30, 2024
21 checks passed
@glehmann
Copy link
Contributor Author

thanks for the kind guidance during the PR!

I'll look at https://github.com/cross-rs/cross-toolchains soon

@Emilgardis Emilgardis added this to the v0.3.0 milestone Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emilgardis Emilgardis Emilgardis approved these changes

Assignees
No one assigned
Labels
A-qemu Area: qemu runners enhancement no-ci-targets PRs that do not affect any cross-compilation targets.
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@glehmann @Emilgardis