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

Docker: Update ossfuzz docker image #11938

Merged
merged 4 commits into from
Sep 15, 2021
Merged

Docker: Update ossfuzz docker image #11938

merged 4 commits into from
Sep 15, 2021

Conversation

bshastry
Copy link
Contributor

The update does the following

@bshastry bshastry enabled auto-merge September 13, 2021 08:17
@bshastry
Copy link
Contributor Author

The failing build should be fixed once #11943 and #11941 have been merged and this PR rebased on them.

@bshastry bshastry force-pushed the update-ossfuzz-docker branch from f7b5038 to 08a7eac Compare September 14, 2021 14:11
@github-actions
Copy link

solbuildpackpusher/solidity-buildpack-deps:ubuntu1604.clang.ossfuzz-12 [solbuildpackpusher/solidity-buildpack-deps@sha256:b949329bce903530a4bd788a244c78b6ed2d8a3163f11bd8829508a0054e18ad].

@@ -22,7 +22,7 @@
# (c) 2016-2019 solidity contributors.
#------------------------------------------------------------------------------
FROM gcr.io/oss-fuzz-base/base-clang as base
Copy link
Member

Choose a reason for hiding this comment

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

I guess this pulls latest base-clang because there's no tag used here? I think we should use a tag with a specific version - to prevent this layer from being taken from cache when we want version to change and also to make it obvious that we're relying on a specific version that should be bumped from time to time.

Copy link
Contributor Author

@bshastry bshastry Sep 15, 2021

Choose a reason for hiding this comment

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

This is a good point. I notice that Google uses the tag latest to refer to the most recent base-clang image. I'm not sure why if pull by tag, even the tag is cached or if the newer tag is fetched.

FROM gcr.io/oss-fuzz-base/base-clang:latest as base

So, if the latest one month later points to an image with a different digest, the change may not be fetched (or reflected in the dockerfile).

As a solution to make it explicit we rely on a specific version, we could pull by digest. Pulling the current digest would look like

FROM gcr.io/oss-fuzz-base/base-clang@sha256:a964dcd3a66bd3898e82869c720a74553d7fe77cfa5d0b87618d59ec7d4f0314 as base

List of base-clang images hosted by Google here: https://console.cloud.google.com/gcr/images/oss-fuzz-base/GLOBAL/base-clang

Notice that they are updated almost on a daily basis!

Copy link
Member

Choose a reason for hiding this comment

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

ok, looks like they are not tagging them with versions.

I think that our CI builds without cache so it's not a big issue. Might be a bigger one if image was meant to be built locally. The bigger problem for me is that the base image really gets updated on any modification to the Dockerfile but it's not very obvious. It gets updated even if you just bump the version. Hard-coding a hash is not great because it's tedious to update and it does not really tell you what you are updating to. So I guess not much we can do here unless Google decides to start tagging it.

In this case I think I'd just add latest to make this more obvious.

bshastry and others added 2 commits September 15, 2021 12:57
Co-authored-by: Kamil Śliwak <kamil.sliwak@codepoets.it>
Co-authored-by: Kamil Śliwak <kamil.sliwak@codepoets.it>
@github-actions
Copy link

solbuildpackpusher/solidity-buildpack-deps:ubuntu1604.clang.ossfuzz-12 [solbuildpackpusher/solidity-buildpack-deps@sha256:a76c6ef3bf25cac5fbe67bd34b64d5a79bcfcec75ecb8abf2a6a2e0f653e9bbd].

@github-actions
Copy link

solbuildpackpusher/solidity-buildpack-deps:ubuntu1604.clang.ossfuzz-12 [solbuildpackpusher/solidity-buildpack-deps@sha256:eb3306173168e3abeb64e2005016cea9f767cd246286c27eb26bfac72073f204].

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I'd add latest but looks find apart from that.

@bshastry bshastry merged commit febbd8d into develop Sep 15, 2021
@bshastry bshastry deleted the update-ossfuzz-docker branch September 15, 2021 16:11
@cameel
Copy link
Member

cameel commented Sep 15, 2021

What about a PR updating .circleci/config.yml?

@bshastry
Copy link
Contributor Author

I'd add latest but looks find apart from that.

This PR got merged before I could update it. Will create a small PR for it later.

What about a PR updating .circleci/config.yml?

Will do.

@cameel
Copy link
Member

cameel commented Sep 16, 2021

This PR got merged before I could update it. Will create a small PR for it later.

Well, at this point I'd leave it as is. Rebuilding these images is a hassle and I think it's not important enough to go through that again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

evmone-standalone static library clang build fails while llvm-ar reads mri script
2 participants