Skip to content

Dockerfile: variant support in frozen-images stage#44114

Merged
thaJeztah merged 1 commit intomoby:masterfrom
crazy-max:frozen-script-variant
Nov 18, 2022
Merged

Dockerfile: variant support in frozen-images stage#44114
thaJeztah merged 1 commit intomoby:masterfrom
crazy-max:frozen-script-variant

Conversation

@crazy-max
Copy link
Member

@crazy-max crazy-max commented Sep 8, 2022 •
edited
Loading

closes #44005
related #43529
needs #44034 (for testing)

- What I did

using TARGETVARIANT in frozen-images stage implies changes in download-frozen-image-v2.sh script to add support for variants so we are able to build against more platforms.

looking at others issues in this repo, this script seems to be used outside the Dockerfile. I didn't check this case. If it's relevant to also check that then I think using scopeo in the Dockerfile is a better alternative: #44005

- How I did it

- How to verify it

$ docker buildx build --platform linux/arm/v7 --target frozen-images .

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@crazy-max crazy-max changed the title Dockerfile: use TARGETVARIANT for frozen images script Dockerfile: variant support in frozen-images stage Sep 8, 2022
@crazy-max crazy-max force-pushed the frozen-script-variant branch 2 times, most recently from 26192c7 to 65d7bff Compare September 8, 2022 13:59
@crazy-max crazy-max marked this pull request as ready for review September 8, 2022 14:00
thaJeztah
thaJeztah reviewed Sep 8, 2022
View reviewed changes
thaJeztah
thaJeztah reviewed Sep 8, 2022
View reviewed changes
@crazy-max crazy-max force-pushed the frozen-script-variant branch 3 times, most recently from 2cc1f4d to 5d2ce80 Compare September 8, 2022 16:56
@tianon
Copy link
Member

tianon commented Sep 8, 2022

The problem with using uname -m for platform detection in general is that it detects the architecture of the kernel, not the architecture of userspace (which is normally what we're actually after).

@neersighted neersighted mentioned this pull request Sep 15, 2022
Closed
@corhere corhere mentioned this pull request Oct 24, 2022
Closed
@corhere
Copy link
Contributor

corhere commented Oct 24, 2022

The problem with using uname -m for platform detection in general is that it detects the architecture of the kernel, not the architecture of userspace (which is normally what we're actually after).

I've looked, but I could not find any distro-independent ways to enumerate the supported ABI personalities of a Linux system. Not even setarch(8) from util-linux has that ability. (setarch --list just dumps the static list compiled into the command without querying whether the kernel supports any of them.) While uname -m may not be perfect, is it a blocker? The $TARGETARCH escape hatch can be used when necessary and we could always improve upon our target arch autodetection later.

@tianon
Copy link
Member

tianon commented Oct 25, 2022

I guess what I'm trying to say is that I don't understand why this PR touches the (already working) get_target_arch function at all. It doesn't seem relevant to adding support for variants to the script.

corhere
corhere requested changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

I guess what I'm trying to say is that I don't understand why this PR touches the (already working) get_target_arch function at all. It doesn't seem relevant to adding support for variants to the script.

Good point! Now that you've pointed it out, I wholeheartedly agree. The modifications to the get_target_arch function are independent and should be split off into a separate PR.

@crazy-max crazy-max force-pushed the frozen-script-variant branch from 5d2ce80 to f6737e2 Compare November 17, 2022 13:56
@crazy-max
Copy link
Member Author

crazy-max commented Nov 17, 2022

Rebased and revert changes to get_target_arch.

@crazy-max crazy-max force-pushed the frozen-script-variant branch from f6737e2 to fa6e5f7 Compare November 17, 2022 16:47
corhere
corhere approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

I have a couple of non-blocking nitpicks

@crazy-max
Dockerfile: variant support in frozen-images stage
25dc760 
using TARGETVARIANT in frozen-images stage implies changes in
`download-frozen-image-v2.sh` script to add support for variants
so we are able to build against more platforms.

Signed-off-by: CrazyMax <[email protected]>
@crazy-max crazy-max force-pushed the frozen-script-variant branch from fa6e5f7 to 25dc760 Compare November 17, 2022 19:57
@thaJeztah thaJeztah added this to the v-next milestone Nov 18, 2022
thaJeztah
thaJeztah approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 8bb5815 into moby:master Nov 18, 2022
@crazy-max crazy-max deleted the frozen-script-variant branch November 18, 2022 14:41
@crazy-max crazy-max mentioned this pull request Nov 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corhere corhere corhere approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/contrib area/packaging process/cherry-picked status/2-code-review

Projects

None yet

Milestone

24.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@crazy-max @tianon @corhere @thaJeztah