-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Add ubuntu 2404 images to CI config #15422
Conversation
3c67d5f
to
6f548be
Compare
ubuntu-2404-docker-image: | ||
type: string | ||
# solbuildpackpusher/solidity-buildpack-deps:ubuntu2404-1 | ||
default: "solbuildpackpusher/solidity-buildpack-deps@sha256:5d6d27551104321a30326d6024bd4e96d9d899a97a78eb9feea364996f1d18b5" |
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.
Just to let it documented, this image comes from: #15321 (comment)
ubuntu-2404-clang-docker-image: | ||
type: string | ||
# solbuildpackpusher/solidity-buildpack-deps:ubuntu2404.clang-1 | ||
default: "solbuildpackpusher/solidity-buildpack-deps@sha256:8df0086907cc1e57068ad48841f2284a87bfbd0a95006286a19a7132d84bb861" |
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 this image comes from here: #15370 (comment)
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.
By the way, I think it's a good convention to hide the comments with older hashes we ended up not using by marking them as "outdated".
- base_ubuntu2404_clang_large: &base_ubuntu2404_clang_large | ||
<<: *base_ubuntu2404_clang | ||
resource_class: large | ||
environment: &base_ubuntu2404_clang_large_env | ||
<<: *base_ubuntu2404_clang_env | ||
MAKEFLAGS: -j 5 | ||
CPUs: 5 | ||
|
||
- base_ubuntu2404_small: &base_ubuntu2404_small | ||
<<: *base_ubuntu2404 | ||
resource_class: small | ||
environment: &base_ubuntu2404_small_env | ||
<<: *base_ubuntu2404_env | ||
MAKEFLAGS: -j 2 | ||
CPUs: 2 | ||
|
||
- base_ubuntu2404_large: &base_ubuntu2404_large | ||
<<: *base_ubuntu2404 | ||
resource_class: large | ||
environment: &base_ubuntu2404_large_env | ||
<<: *base_ubuntu2404_env | ||
MAKEFLAGS: -j 5 | ||
CPUs: 5 | ||
|
||
- base_ubuntu2404_xlarge: &base_ubuntu2404_xlarge | ||
<<: *base_ubuntu2404 | ||
resource_class: xlarge | ||
environment: &base_ubuntu2404_xlarge_env | ||
<<: *base_ubuntu2404_env | ||
MAKEFLAGS: -j 10 | ||
CPUs: 10 |
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 generally add and/remove variants other than the default one as needed. So no need to add them already if nothing's using them.
Having them does not hurt, but the config is already quite long :)
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.
Oh, I guess the assumption is that we'll switch all the 2204 ones to them anyway so they'll all be in use soon?
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.
Correct - the first step is to merge this PR. Then @rodiazet will rebase his evmone bump PR and switch a few of the failing soltest jobs to use the 2404 images added here (required for the evmone bump to pass). Once his PR is merged, we can then switch the rest of the 2204 jobs to 2404, and perform any required clean-up (i.e. delete the 2204 images).
@@ -19,6 +19,14 @@ parameters: | |||
type: string | |||
# solbuildpackpusher/solidity-buildpack-deps:ubuntu2204.clang-8 | |||
default: "solbuildpackpusher/solidity-buildpack-deps@sha256:2662376fe0e1ec2d346495a19a6d64508c1048d5a7325d8600c33c343fa64a0f" |
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.
Why was #15321 even merged without having a matching PR updating CI that passes all checks? Because of this we're in a really weird state now where the dockerfile source does not match what we're running and also it blocks us from doing any other changes to the images that we might need in the meantime (at least without reverting them). Good thing that we apparently did not need any so far.
Unless these are going to be bumped or removed very soon, we should really revert these images (with a version bump to avoid reusing the number).
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 you mean? #15320
We had a ton of problems with the new evmone bump (and still do for ossfuzz) while you were off.
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.
The idea here is to introduce the 2404 images to circleCI config (without actually making use of them). @rodiazet is then to make use of these new images in #15320, and only for the 3 or so failing jobs (soltest, force release, etc.), which will allow the evmone bump to pass all of the tests and be merged. We can then update the rest of the jobs to make use of 2404 images wherever appropriate, but in another PR.