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.

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

feat: build image by platform #1582

Merged
merged 7 commits into from
May 12, 2023
Merged

Conversation

n063h
Copy link
Contributor

@n063h n063h commented Apr 28, 2023

refer to #1580

Before

Hardcoded linux/amd64 platform into code, so envd builds image on linux/amd64 platform by default.

After

support --platform flag to build image on target platform.

@n063h n063h marked this pull request as draft April 28, 2023 09:26
@kemingy kemingy linked an issue Apr 28, 2023 that may be closed by this pull request
@pingsutw
Copy link
Member

pingsutw commented May 4, 2023

@n063h Thanks for working on this. Any update for it. we need muti-arch image. I can help for testing

@n063h
Copy link
Contributor Author

n063h commented May 5, 2023

@n063h Thanks for working on this. Any update for it. we need muti-arch image. I can help for testing

@pingsutw Thanks for reaching out! There's a small hiccup with the dependency "horust" image that I'm working on fixing. I'll get it sorted out soon.

@n063h n063h force-pushed the build-image-by-platform branch from 44f3e39 to d7bc210 Compare May 6, 2023 17:38
@n063h
Copy link
Contributor Author

n063h commented May 6, 2023

@pingsutw Hi, sorry for the wait, I think this branch is ready for your test now. You can run envd build --platform linux/amd64,linux/arm64 to build multi-platform images.
Since I have no machine on Non-x86 platforms, I tested it with qemu on linux/arm64 which worked well.
Please note: horust now provides only linux/amd64,linux/arm64 images, so the images built from envd also work just for the two platforms.

@kemingy
Copy link
Member

kemingy commented May 6, 2023

Is it ready to review?

BTW the dev env only work for aarch64/amd64, but the non-dev env should work for more platforms.

@n063h n063h marked this pull request as ready for review May 7, 2023 04:46
@n063h
Copy link
Contributor Author

n063h commented May 7, 2023

Is it ready to review?

BTW the dev env only work for aarch64/amd64, but the non-dev env should work for more platforms.

yes.
And the default target-platform is "linux/amd64" no matter what the build-platform is, which may require explicitly flag for arch arm64 for macos users.

gaocegege
gaocegege previously approved these changes May 8, 2023
Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/cc @kemingy

pkg/app/build.go Outdated Show resolved Hide resolved
pkg/app/build.go Outdated Show resolved Hide resolved
pkg/app/build.go Outdated Show resolved Hide resolved
@kemingy
Copy link
Member

kemingy commented May 8, 2023

LGTM. Please resolve the conflicts.

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

could we also update the platform for NewImageSource

sys := types.SystemContext{}
if platform != nil {
  sys.ArchitectureChoice = platform.Architecture
  sys.OSChoice = platform.OS
}
src, err := ref.NewImageSource(ctx, &sys)

@n063h
Copy link
Contributor Author

n063h commented May 11, 2023

could we also update the platform for NewImageSource

sys := types.SystemContext{}
if platform != nil {
  sys.ArchitectureChoice = platform.Architecture
  sys.OSChoice = platform.OS
}
src, err := ref.NewImageSource(ctx, &sys)

What is the purpose of these changes,a re there any issues with the current code? @pingsutw

@pingsutw
Copy link
Member

It will pull the base image. I'm using envd in my macbook. It still tries to pull darwin/amd64 base image, although I set the platform to linux/amd64, so we also need to update the platform for NewImageSource.
#1587

@n063h
Copy link
Contributor Author

n063h commented May 11, 2023

It will pull the base image. I'm using envd in my macbook. It still tries to pull darwin/amd64 base image, although I set the platform to linux/amd64, so we also need to update the platform for NewImageSource. #1587

I got you, the update has been pushed.

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@pingsutw
Copy link
Member

btw, @kemingy could you help me cut a release after it merges? I need to bump envd version in flytekit.

@kemingy kemingy added this pull request to the merge queue May 12, 2023
@kemingy kemingy removed this pull request from the merge queue due to a manual request May 12, 2023
@kemingy kemingy merged commit 6569021 into tensorchord:main May 12, 2023
@kemingy
Copy link
Member

kemingy commented May 12, 2023

btw, @kemingy could you help me cut a release after it merges? I need to bump envd version in flytekit.

https://github.com/tensorchord/envd/releases/tag/v0.3.22 is released

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.

feat: Support a new CLI flag --platform
4 participants