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

Workaround for Boost compilation error with Clang 18 and libc++ in ossfuzz buildpack-deps #15713

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Jan 14, 2025

This PR implements the same workaround applied by the ossfuzz maintainers to the Solidity ossfuzz setup:

https://github.com/google/oss-fuzz/blob/5a9322260c4c66e7eb3e5d922b9fee6621ffd8da/projects/solidity/Dockerfile#L17-L18

It replaces base-clang with base-builder, the same image used by ossfuzz, that utilizes Clang 15. This change allows us to continue running our instance in CI until a fix for the main issue is available.

Workaround for: #15711

@@ -59,26 +62,19 @@ RUN apt-get update; \
pygments-lexer-solidity \
pylint \
requests \
tabulate \
z3-solver;
Copy link
Member Author

Choose a reason for hiding this comment

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

Z3 is already installed from source in the steps below, so this is unnecessary. I also removed CMake, as it is already included in the base image.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the python package of Z3, which is unrelated to the Z3 binary built below.
However, if it can be removed without anything breaking, that it's good to remove it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it is not used by any CI job based on the ossfuzz image as far as I know. The chk_proofs which is the job that runs the script mentioned here: #15551 (comment) uses the Ubuntu 24.04 image and it is not run by ossfuzz. This is why I'm also wondering if all the smt stuff is really needed here, or if we should just remove it from this image.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I updated Z3 and CVC5, but I kept the removal of the z3-python, I don't think it is relevant for this image. Because it is required by the tests under test/formal, which indeed uses the z3 python bindings, but I believe the fuzzer does not run such tests. Maybe there are more things that we could cleanup as well.

This comment was marked as outdated.

LABEL version="8"
# FIXME: Workaround for Boost 1.83.0 build failure when using Clang 18 (default since https://github.com/google/oss-fuzz/pull/11714)
# See: https://github.com/google/oss-fuzz/blob/5a9322260c4c66e7eb3e5d922b9fee6621ffd8da/projects/solidity/Dockerfile#L17C1-L18C139
FROM gcr.io/oss-fuzz-base/base-builder@sha256:19782f7fe8092843368894dbc471ce9b30dd6a2813946071a36e8b05f5b1e27e AS base
Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @bshastry do you see any problem in switch images here?

@r0qs r0qs mentioned this pull request Jan 15, 2025
@r0qs r0qs requested review from nikola-matic and clonker January 15, 2025 15:05
nikola-matic
nikola-matic previously approved these changes Jan 15, 2025
Copy link
Collaborator

@nikola-matic nikola-matic left a comment

Choose a reason for hiding this comment

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

Soft approve. Let's wait until end of week for @bshastry to chime in regarding the base image change.

@r0qs
Copy link
Member Author

r0qs commented Jan 15, 2025

Soft approve. Let's wait until end of week for @bshastry to chime in regarding the base image change.

Sure, better to have a response from him on that. Also, honestly, we could maybe even cleanup a bit more our current image. Do we fuzz with smt enabled?

@blishko
Copy link
Contributor

blishko commented Jan 15, 2025

If you end up not removing the solvers, then they should be updated as was done for other Docker images in #15551.
I deliberately did not touch ossfuzz image there because of the failing build.

@r0qs r0qs force-pushed the fix-ossfuzz-build branch 2 times, most recently from 61b3046 to 2278b26 Compare January 17, 2025 13:40
@r0qs r0qs force-pushed the fix-ossfuzz-build branch 2 times, most recently from 4d41e5d to 7d91183 Compare January 17, 2025 14:07
@r0qs r0qs force-pushed the fix-ossfuzz-build branch from 7d91183 to baadd1a Compare January 17, 2025 14:15
Copy link

solbuildpackpusher/solidity-buildpack-deps:ubuntu.clang.ossfuzz-9 [solbuildpackpusher/solidity-buildpack-deps@sha256:786fb28bad41835755ac9758bb1f5a15a257ed22c995dd595b1c516ffd4581ff].

@bshastry
Copy link
Contributor

LGTM if build passes :-)

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

Successfully merging this pull request may close these issues.

4 participants