-
Notifications
You must be signed in to change notification settings - Fork 214
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
[FIX] Don't crash updating UID/GID when the image's platform is different from the native CPU arch #746
[FIX] Don't crash updating UID/GID when the image's platform is different from the native CPU arch #746
Conversation
CI appears to fail because no emulator is registered. It works for me on an AMD64 host with an ARM64 image. Could you append the log from the failure? I'm using the following for registering the emulator:
|
I have the emulator installed locally and see an error building an ARM64 image from an AMD64 host:
Here's
|
I'm also able to replicate this error in isolated CI environments where the I get more info when I try to set
|
@chrmarti I just pushed a commit that fixes the test for me locally. The fix is a bit complicated, so I had to touch a few different places:
I started at step 6 and worked my way backwards until my new tests passed, so I'm sure I didn't update everywhere that needs it. |
The CI test failures seem to be temporary apt repo/network issues:
Hopefully re-running should resolve them. |
…gistry calls, add Os and Architecture properties to ImageDetails, and pass --platform in updateRemoteUserUID
97f8abe
to
c4d0ec3
Compare
Oops, didn't mean to bring it out of draft state. It looks promising on first glance, I have triggered CI now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left a few comments. Part of the complexity seems to come from the fact that we might not have pulled the base image yet, otherwise we could just always inspect that and wouldn't have to pass the platform around?
We're inspecting the image first (step 3 below), but we were ignoring its
|
…ssed as build option and run args
@chrmarti I just pushed a commit to also handle the variant component of platforms like I'm not sure how to get the variant of the host platform without exec'ing I also added tests that pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I'll ask for a second reviewer now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
If the user changes the platform to a non-native arch as suggested here, the devcontainers CLI fails to launch due to attempting to load the wrong platform during the "updateUID" image build.
Opening as a draft PR to show the behavior. Will follow up with a fix once I figure out where it should go.