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

Refactor into a base image. #953

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Alexhuszagh
Copy link
Contributor

No description provided.

@Alexhuszagh Alexhuszagh added the meta issues/PRs related to the maintenance of the crate. label Jul 16, 2022
@Alexhuszagh
Copy link
Contributor Author

This still needs to fix deployment and CI: we need to make sure the base image is always built first, and that it is staged for our other image build. It also needs to then be deployed as ghcr.io/cross-rs/base:$tag. The other solution would be to use templates: we run a script which would configure every image, which would avoid deploying another image. But I'm not sure I like that solution.

@Alexhuszagh Alexhuszagh added the no changelog A valid PR without changelog (no-changelog) label Jul 16, 2022
xtask/src/util.rs Outdated Show resolved Hide resolved
@Alexhuszagh Alexhuszagh force-pushed the base_image branch 2 times, most recently from d61b78f to 226b807 Compare September 30, 2022 13:32
@Alexhuszagh
Copy link
Contributor Author

This will probably fail, but I'm wondering to see how the CI orders things so we can use the proper base image.
bors try --target x86_64-unknown-linux-gnu

@bors
Copy link
Contributor

bors bot commented Sep 30, 2022

try

Build failed:

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Sep 30, 2022

As expected, this is failing because ghcr.io/cross-rs/ubuntu:main doesn't exist.
https://github.com/cross-rs/cross/actions/runs/3159167477/jobs/5142204364#step:10:95

#3 [internal] load metadata for ghcr.io/cross-rs/ubuntu:main
#3 ERROR: failed to authorize: failed to fetch anonymous token: unexpected status: 403 Forbidden
------
 > [internal] load metadata for ghcr.io/cross-rs/ubuntu:main:
------
Dockerfile.x86_64-unknown-linux-gnu:1
--------------------
   1 | >>> FROM ghcr.io/cross-rs/ubuntu:main
   2 |     ARG DEBIAN_FRONTEND=noninteractive
   3 |     
--------------------
ERROR: failed to solve: failed to fetch anonymous token: unexpected status: 403 Forbidden

We either need to do this incrementally (IE, deploy once then have our builds rely on that, in which case our builds would be out-of-sync with the base images, which would be bad), or need to build and stage the image from the build-base (which I'd need to figure out how to do in CI).

The first option would be bad because if we upgrade our base to say, Ubuntu 22.04, then our first build would use 20.04, not 22.04 since it would use ubuntu:main from the previous build. Any subsequent builds would work after the first merge, but this would be a bad design.

I'm guessing one solution is to save to a tarball and then load from there? I'll look into doing this.

@Emilgardis
Copy link
Member

Emilgardis commented Sep 30, 2022

I feel like there should be a GH action already made for saving an image to artifacts and then loading it

@Alexhuszagh
Copy link
Contributor Author

Will still fail, just testing saving to tarball.
bors try --target x86_64-unknown-linux-gnu

bors bot added a commit that referenced this pull request Sep 30, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

1 similar comment
@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Sep 30, 2022
@bors

This comment was marked as outdated.

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

I think I personally prefer having these as

ghcr.io/cross-rs/base:ubuntu-vxxx

less "waste" so to say

.github/workflows/ci.yml Outdated Show resolved Hide resolved

- name: Save Docker Image
id: save-docker-image
run: docker save "${IMAGE}:${TAG}" -o "${IMAGE}-${TAG}-${SUB}.tar"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our docker load step always needs to load these as main, since that's what we use for our Dockerfiles expect for the base images:

FROM ghcr.io/cross-rs/base:emscripten-main

We technically don't have to deploy these images, but it might be nice for people trying to build new images based off this logic locally, especially cross maintainers.

Our docker load step should also only load the necessary base for our image: centos, ubuntu, emscripten, etc.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Oct 4, 2022
…inux-gnu.centos --target wasm32-unknown-emscripten
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Oct 4, 2022
…inux-gnu.centos --target wasm32-unknown-emscripten
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Oct 4, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Oct 4, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Oct 4, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh
Copy link
Contributor Author

It seems that the path is the directory for downloading artifacts, not the file name... This should be fixable.

$ ls -la base.tar
drwxr-xr-x  2 runner docker       4096 Oct  4 19:30 .
drwxr-xr-x 15 runner docker       4096 Oct  4 19:30 ..
-rw-r--r--  1 runner docker 1001731072 Oct  4 19:30 base-main-ubuntu.tar

@Alexhuszagh
Copy link
Contributor Author

bors try --target x86_64-unknown-linux-gnu

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Oct 4, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

@Alexhuszagh
Copy link
Contributor Author

bors try-
bors try --target x86_64-unknown-linux-gnu

bors bot added a commit that referenced this pull request Oct 4, 2022
@bors
Copy link
Contributor

bors bot commented Oct 4, 2022

try

Build failed:

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Oct 4, 2022

EDIT: It's the --pull flag that's the issue.

Is.... the docker images within the same job not preserved between steps?

https://github.com/cross-rs/cross/actions/runs/3185987473/jobs/5196287429#step:12:14

Load Base Image

Loaded image: ghcr.io/cross-rs/base:main-ubuntu

Build Docker Image

#3 [internal] load metadata for ghcr.io/cross-rs/base:main-ubuntu
#3 ERROR: failed to authorize: failed to fetch anonymous token: unexpected status: 403 Forbidden
------
 > [internal] load metadata for ghcr.io/cross-rs/base:main-ubuntu:
------
Dockerfile.x86_64-unknown-linux-gnu:1
--------------------
   1 | >>> FROM ghcr.io/cross-rs/base:main-ubuntu
   2 |     ARG DEBIAN_FRONTEND=noninteractive
   3 |     
--------------------
ERROR: failed to solve: failed to fetch anonymous token: unexpected status: 403 Forbidden

I guess I can convert load and build to a single step then.

@Emilgardis
Copy link
Member

Emilgardis commented Oct 7, 2022

No it 100% should work, otherwise we wouldnt be able to do our tests

This is probably the issue I had with buildx, you need to specify the output

docker_build.args(["--output", "type=docker"]);

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Oct 7, 2022

No it 100% should work, otherwise we wouldnt be able to do our tests

I actually found it, it's the --pull flag where there's no registry entry. Since we never need this in CI, I've added an environment variable to disable it and set it to true in our CI.

@Alexhuszagh
Copy link
Contributor Author

bors try --target x86_64-unknown-linux-gnu

bors bot added a commit that referenced this pull request Oct 7, 2022
@bors
Copy link
Contributor

bors bot commented Oct 7, 2022

try

Build failed:

@Emilgardis
Copy link
Member

thought it could be this #817 (comment)

but maybe not.

@Emilgardis
Copy link
Member

Emilgardis commented Oct 7, 2022

@Emilgardis Emilgardis mentioned this pull request Apr 9, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta issues/PRs related to the maintenance of the crate. no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants