-
Notifications
You must be signed in to change notification settings - Fork 397
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 image build errors with podman and nerdctl. #1033
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
6e272fa
to
9e73a76
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
bors try --target x86_64-unknown-linux-gnu I've added a custom image build to the Podman test (it's a trivial pre-build step, so it just tests the image build steps work), so we can ensure building and testing images work. |
tryBuild succeeded: |
32c3970
to
3865429
Compare
I haven't added tests for Lima ( |
We might also want to default to no |
3865429
to
3a960e4
Compare
3a960e4
to
e1ccaa6
Compare
@@ -187,7 +187,7 @@ pub fn build_docker_image( | |||
|
|||
if push { | |||
docker_build.arg("--push"); | |||
} else if engine.kind.is_docker() && no_output { | |||
} else if engine.kind.supports_output_flag() && no_output { |
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.
I feel like this should be an error, i.e it should not be possible to specify no output and then not have it happen
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.
I've changed it so we error if it doesn't support the output flag, with another branch:
if push {
docker_build.arg("--push");
} else if engine.kind.supports_output_flag() && no_output {
docker_build.args(["--output", "type=tar,dest=/dev/null"]);
} else if no_output {
msg_info.fatal("cannot specify `--no-output` with engine that does not support the `--output` flag", 1);
} else if has_buildkit {
docker_build.arg("--load");
}
Ensures that Docker and NerdCTL, which support the custom `--output` and `--cache-from` flags use them, while Podman and unknown container engines use the strict, minimal subset Podman supports. This is because Podman only supports a registry/repository, without a tag, for `--cache-from`, which means it synchronizes poorly with our target subs, like `centos`. Therefore, unless unsupported, we should always use the features available for our container engine. This also fixes the `--pull` flag on NerdCTL, which is unsupported.
e1ccaa6
to
e5d58cc
Compare
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!
bors r+
Build succeeded: |
Ensures that docker and nerdctl, which support the custom
--output
and--cache-from
flags use them, while podman and unknown container engines use the strict, minimal subset podman supports. This is because podman only supports a registry/repository, without a tag, for--cache-from
, which means it synchronizes poorly with our target subs, likecentos
. Therefore, unless unsupported, we should always use the features available for our container engine.This also fixes the
--pull
flag on nerdctl, which is unsupported. Engines without BuildKit extensions by default, such as nerdctl, now do not usebuildx
unless explicitly enabled withCROSS_CONTAINER_ENGINE_NO_BUILDKIT=0
.Closes #1031.