-
Notifications
You must be signed in to change notification settings - Fork 386
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
allow specifying a dockerfile to use for a target and implement "pre-build"ing #678
Conversation
src/docker.rs
Outdated
@@ -248,8 +250,51 @@ pub fn run( | |||
} | |||
} | |||
|
|||
let image = if let Some(dockerfile) = config.dockerfile(target)? { | |||
let mut docker_build = docker_command("build")?; |
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.
Should maybe add a flag to allow specifying more flags for this command
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.
We already have build-args
, so we need anything else?
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.
thinking like DOCKER_OPTS
, I think a DOCKER_BUILD_OPTS
is the best way to do it
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.
Ah ok, that's a good suggestion. Maybe also consider then #770, since we handle more than just docker now? If we're introducing new environment variables, we don't need to worry about backwards compatibility.
afd44c2
to
14c7ff8
Compare
I think this is a nice addition to #635 |
2bf937d
to
876fcbd
Compare
We can also add a label to our images to signal that an image is intended for cross, and that way we can return those with the tools in #767 not sure what to label them though, |
src/docker.rs
Outdated
@@ -248,8 +250,51 @@ pub fn run( | |||
} | |||
} | |||
|
|||
let image = if let Some(dockerfile) = config.dockerfile(target)? { | |||
let mut docker_build = docker_command("build")?; |
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.
We already have build-args
, so we need anything else?
src/docker.rs
Outdated
} | ||
|
||
match self { | ||
Dockerfile::File { path, .. } => { |
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.
Would it be preferable to handle the FROM {{image}}
ourselves? That way the users can just add the components they want afterwards? Not sure about this but I think it might be easier.
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 don't understand this comment I think.
do you mean for pre-build, we do something like
FROM {image}
{instructions}
where {instructions}
is the entries newline separated?
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.
Yeah, say we have a Dockerfile as follows for bindgen
support:
RUN apt-get update && apt-get install --assume-yes --no-install-recommends \
libclang-3.9-dev clang-3.9
We want to use the same base Dockerfile for all images, so we'd rather not have to create a new Dockerfile for all targets. For the target armv7-unknown-linux-gnueabihf
, this will automatically get transformed to:
FROM ghcr.io/cross-rs/armv7-unknown-linux-gnueabihf:main
RUN apt-get update && apt-get install --assume-yes --no-install-recommends \
libclang-3.9-dev clang-3.9
And this way all we need to do is ensure the name is provided in our Cross.toml
for all targets. Also, then we could concatenate the pre-build steps and this into a single file and avoid having to add the pre-build
tag. If we also have pre-build hooks, this would then become:
FROM ghcr.io/cross-rs/armv7-unknown-linux-gnueabihf:main
RUN apt-get update && apt-get install --assume-yes --no-install-recommends \
libclang-3.9-dev clang-3.9
ARG CROSS_CMD
RUN eval "${{CROSS_CMD}}"
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 fail to see the point, you could already do that with
pre-build = ["apt-get update && apt-get install --assume-yes --no-install-recommends libclang-3.9-dev clang-3.9"]
I see another solution though, make a target.<target>.dockerfile.provide-base
that would do it instead with the image you provide. But I still don't see the benefit
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.
That's just a simple example: if something requires the ENV
keyword or similar (like, say we want vcpkg
for providing dependencies of cross-compiled C/C++ libraries), we can't simply use pre-build hooks.
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.
A more complex example would be:
RUN apt-get update && apt-get install --assume-yes --no-install-recommends \
libclang-3.9-dev clang-3.9
RUN git clone https://github.com/Microsoft/vcpkg.git && ./vcpkg/bootstrap-vcpkg.sh
ENV PATH /path/to/vcpkg/bin/dir/:$PATH
# do more stuff here, maybe add an ARG, etc.
# everything is platform generic.
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.
Maybe we could have pre-build
hooks encompass this functionality (RUN
isn't implicit anymore, and then all this can be easily encompassed there)? That way you can provide a complete Dockerfile, or just add extra commands to the existing one?
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 implemented this (i think, not fully tested), but with allowing the user to tell cross to insert FROM
with target.TARGET.dockerfile.provide-base
.
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.
and removed with f98a6e6
instead we provide a build arg
ARG CROSS_BUILD_IMAGE
that will be whatever cross would use
I think that's a good idea, and we can then add extracting the label if needed for the list/remove images commands. |
f198b74
to
e56d9be
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.
Would it make sense to allow the following
[target.aarch64-unknown-linux-gnu.pre-build]
lines = ["/lib.sh"]
[[target.aarch64-unknown-linux-gnu.pre-build.copy]]
files = ["scripts/lib.sh"]
location = "/"
ba7a22c
to
92db711
Compare
1a0a942
to
3c7f900
Compare
this should be ready now, I'm happy with the functionality. |
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.
Minor comments, otherwise looks great.
content: format!( | ||
r#" | ||
FROM {image} | ||
ARG CROSS_DEB_ARCH= |
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.
This (CROSS_DEB_ARCH
) should probably be added to the wiki?
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 added it to the readme, will definitely add it to the wiki
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.
scratch that, I haven't... wiki seems good
d113ac1
to
0bfc338
Compare
all done! |
Went over all the changes from the comments, and it looks good. Once this merges I'll be adding additional documentation to the wiki. bors r+ |
Build succeeded: |
Add optional
target.{target}.dockerfile[.file]
,target.{target}.dockerfile.context
andtarget.{target}.dockerfile.build-args
to invokedocker/podman build
before using an image.also adds
target.{target}.pre-build
to allow pre-building as per #565In total, this PR allows a user to do the following:
fixes #565