Use linux-micro sandbox image for local development #1360
Conversation
Previously, only local images could be overridden. This adds a new env variable DOCSRS_DOCKER_IMAGE, and makes the existing variable DOCS_RS_LOCAL_DOCKER_IMAGE a synonym for it. At some point I want to remove the old name, but that requires updating forge first.
jyn514
added
A-newcomer-roadblock
| let image = match SandboxImage::local(&custom_image) { | ||
| Ok(i) => i, | ||
| Err(CommandError::SandboxImageMissing(_)) => SandboxImage::remote(custom_image)?, | ||
| Err(err) => return Err(err.into()), | ||
| }; |
There was a problem hiding this comment.
I don't think this is what we want: this would result in the image never being updated, as once the image is downloaded it's available locally. A good heuristic could be to treat an image as remote if it contains /.
There was a problem hiding this comment.
A good heuristic could be to treat an image as remote if it contains /.
Hmm, there are plenty of remote images that don't have / though, like ubuntu or alpine. Personally I would be ok with it being cached indefinitely; you can always remove the image if it gets out of date. Is there a way to tell if a local image was built locally or downloaded?
There was a problem hiding this comment.
The full name for ubuntu is docker.io/ubuntu, docker just has a hardcoded "try docker.io if no registry specified". That said, just using a locally cached image without checking that it's up to date is pretty standard behaviour of docker tooling, you either need to explicitly docker pull ... to update it, or pass --pull to commands like docker build to have them attempt pulling updates.
There was a problem hiding this comment.
The full name for ubuntu is docker.io/ubuntu, docker just has a hardcoded "try docker.io if no registry specified".
Well yes, but if someone overrides the image, they'll be using the shorthand.
There was a problem hiding this comment.
Honestly I'm not sure: I don't want us accidentally use an older image in production because we end up changing the environment variable without realizing it.
Hmm, there are plenty of remote images that don't have / though, like ubuntu or alpine.
I don't think that's a problem, you can't realistically build any Rust crate using those images.
There was a problem hiding this comment.
Honestly I'm not sure: I don't want us accidentally use an older image in production because we end up changing the environment variable without realizing it.
Oh I see - I don't think this will have any impact on prod, we don't override the image there.
There was a problem hiding this comment.
If we do want to change the image in prod, I think we should do that by changing the default, not through environment variables.
|
Ooh good idea, let me update that. |
I tested that this can build `hexponent 0.3.0` successfully. This cuts the download size down from over 1 GB to about 150 MB.
jyn514
added
the
S-waiting-on-review
|
@jyn514 are still design-questions unanswered here? For the local/test setup it works fine as it should, with default rustwide image, local image and remote image. |
|
( without the new env set it still uses the old image path, but that's due to rustwide being not updated on this branch. Which will be fine once merged) |
Add support for overriding remote docker images
Previously, only local images could be overridden.
This adds a new env variable DOCSRS_DOCKER_IMAGE, and makes the existing
variable DOCS_RS_LOCAL_DOCKER_IMAGE a synonym for it. At some point I
want to remove the old name, but that requires updating forge first.
Use the linux-micro image for tests
I tested that this can build
hexponent 0.3.0successfully. This cutsthe download size down from over 1 GB to about 150 MB.
Thank you pietro for your hard work on rust-lang/crates-build-env#78!
r? @pietroalbini
cc @rust-lang/docs-rs - you'll want to update your .env files so it's easier to run builds.