Skip to content

Conversation

@r0qs
Copy link
Member

@r0qs r0qs commented Feb 14, 2023

Fixes #13954

The PR restores the ubuntu 20.04 Dockerfile, modifies b_ubu_static to use the old ubuntu 20.04 image, and updates the bytecode comparison to check the static builds and, hopefully, makes @cameel happy.

@r0qs r0qs requested review from cameel and ekpyron February 14, 2023 10:27
ninja; \
ninja install/strip; \
rm -rf /usr/src/evmone

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the Hera build from the original Dockerfile. Not sure if I should include it again. Should I?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea was to just bring it back for reference, s.t. all images used in CI have a dockerfile here explaining what's in them - so in that sense, since that old image has hera in it, it'd be more accurate to keep it in here, unchanged, but it doesn't really matter that much - if we keep this mechanism for the time being, the very next build shouldn't include hera, so also fine to remove it right away :-).

Copy link
Member Author

@r0qs r0qs Feb 14, 2023

Choose a reason for hiding this comment

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

I brought it back in any case. So we keep things consistent as they were :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could do a follow up PR with base image cleanup. We have a few small annoyances that you could fix:

  • Remove Hera.
  • Add all those dependency packages we're installing with apt install in config.yml. They should really be already included in the base image.
  • Rename Dockerfile.ubuntu1604.clang.ossfuzz to Dockerfile.clang.ossfuzz. It's not using Ubuntu 16.04 but whatever version the clang is using.

Also, what about the libcln-dev change we did for 22.04? Is it just missing on newer Ubuntu or is it a general change that we should have on 20.04 too?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Remove Hera.

You mean remove it from the solidity::test? Because I already dropped it in the new ubuntu images.

  • Remove Hera.

  • Add all those dependency packages we're installing with apt install in config.yml. They should really be already included in the base image.

  • Rename Dockerfile.ubuntu1604.clang.ossfuzz to Dockerfile.clang.ossfuzz. It's not using Ubuntu 16.04 but whatever version the clang is using.

But should all those be done in one follow up PR or maybe 3 quick ones for each of the bullet points?

Also, what about the libcln-dev change we did for 22.04? Is it just missing on newer Ubuntu or is it a general change that we should have on 20.04 too?

Ah, it is because CVC4 has CLN as an optional feature but Ubuntu 22.04 ships CVC4 with it enabled by default while Ubuntu 20.04 uses GMP by default. See this comment here: #13891 (comment)
So for the newer Ubuntu versions that uses CVC4 we need libcln-dev installed.

Copy link
Collaborator

@cameel cameel Feb 16, 2023

Choose a reason for hiding this comment

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

You mean remove it from the solidity::test? Because I already dropped it in the new ubuntu images.

From the 20.04 image. Daniel said not to remove it in this PR because we want the source to match the restored image. But the follow up could just rebuild the image without hera and then removing this bit would be appropriate.

But should all those be done in one follow up PR or maybe 3 quick ones for each of the bullet points?

Either is fine. I'd usually suggest keeping PRs as focused as possible but some or even all of them will trigger image rebuild so for these I think it'll be easier to do it as one PR than to have multiple simultaneous rebuilds :)

So for the newer Ubuntu versions that uses CVC4 we need libcln-dev installed.

Ah, ok then.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the 20.04 image. Daniel said not to remove it in this PR because we want the source to match the restored image. But the follow up could just rebuild the image without hera and then removing this bit would be appropriate.

Ah, got it. Will do.

Either is fine. I'd usually suggest keeping PRs as focused as possible but some or even all of them will trigger image rebuild so for these I think it'll be easier to do it as one PR than to have multiple simultaneous rebuilds :)

Good point. Indeed for this specific case I agree that it is better to go for one PR then.

@r0qs r0qs force-pushed the static-build-2004 branch 8 times, most recently from 548bf16 to 5394351 Compare February 14, 2023 22:21
cp workspace/build/solc/solc github/solc-macos
cp workspace/solc/Release/solc.exe github/solc-windows.exe
cp workspace/soljson.js github/soljson.js
cp workspace/solc/solc-static-linux github/solc-static-linux
Copy link
Member Author

Choose a reason for hiding this comment

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

I still need to check if the release workflow works properly.

Copy link
Member Author

@r0qs r0qs Feb 15, 2023

Choose a reason for hiding this comment

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

Just tested in a local fork and it looks like it worked. Here are the test binaries for reference: https://app.circleci.com/pipelines/github/r0qs/solidity/288/workflows/fdfd66dc-d7a8-41a4-864a-8dc494bbf350/jobs/8432/artifacts

I checked the static binary on Ubuntu 20.04, 22.04 and 22.10 and on Archlinux.

ninja; \
ninja install/strip; \
rm -rf /usr/src/evmone

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could do a follow up PR with base image cleanup. We have a few small annoyances that you could fix:

  • Remove Hera.
  • Add all those dependency packages we're installing with apt install in config.yml. They should really be already included in the base image.
  • Rename Dockerfile.ubuntu1604.clang.ossfuzz to Dockerfile.clang.ossfuzz. It's not using Ubuntu 16.04 but whatever version the clang is using.

Also, what about the libcln-dev change we did for 22.04? Is it just missing on newer Ubuntu or is it a general change that we should have on 20.04 too?


- job_native_test_ext_gnosis: &job_native_test_ext_gnosis
<<: *workflow_ubuntu2204_static
<<: *workflow_ubuntu2004_static
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's actually weird that we even include the version here. It's misleading IMO - it looks like it should make the job run on Ubuntu 20.04 but what it really does is just add a dependency on b_ubu_static. It's b_ubu_static that happens to run on 20.04 and that's completely superfluous information. It would also be pretty easy to forget to update the name when moving a job to a new base image.

I think we would be better off renaming these things. My suggestion would be:

  • For the ones that filter branches:
    • workflow_trigger_on_tags -> on_all_tags_and_branches
    • workflow_trigger_on_releases -> on_version_tags
    • A new one for nightly that's currently specified directly: on_develop
  • For the ones that specify dependencies:
    • workflow_ubuntu2004_static -> requires_b_ubu_static
    • workflow_ubuntu2204_asan_clang -> requires_b_ubu_asan_clang
    • ...
    • For ones that do not require anything add requires_nothing which is just on_all_tags_and_branches with no extra fields.

This is out of scope of this PR of course, but you could include this change if you decide to do that base image cleanup PR I mentioned in another comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice. I will do that in the new PR then. But requires_nothing is a bit weird no? Maybe we can just make a default_workflow or something like that.

Copy link
Collaborator

@cameel cameel Feb 16, 2023

Choose a reason for hiding this comment

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

Yeah, it's not great but I don't have a better name. I don't think default_workflow is that nice - the point was to say there are no dependent jobs and I don't think it makes that clear enough. It could also be just on_all_tags_and_branches i.e. the way it is now, but I don't like that too much either for the same reason. It makes it looks like it refers to something completely different than dependencies.

Maybe you have some other name suggestions?

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, no, I don't have a better suggestion, maybe just no_dependencies haha
But yeah, I will use your suggestion in the new PR (i.e. requires_nothing), and we can think on a better name later if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe just no_dependencies haha

I thought about that one too and I'd be fine with it. I just thought that requires_nothing would be more inline with the other requires_xyz dicts we would get after the renaming.

@r0qs r0qs force-pushed the static-build-2004 branch from f2a11d2 to 308e7e3 Compare February 16, 2023 17:09
Copy link
Collaborator

@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.

It's probably ok, but it would be easier to check if some more invasive refactors were moved out to a separate PR :)

@r0qs r0qs force-pushed the static-build-2004 branch from 308e7e3 to 4db41e0 Compare February 17, 2023 16:39
cameel
cameel previously approved these changes Feb 17, 2023
@cameel
Copy link
Collaborator

cameel commented Feb 17, 2023

Looks good. Please squash the fixes into the commits they fix and we can merge.

@r0qs r0qs force-pushed the static-build-2004 branch 2 times, most recently from 68f8213 to 6b564e0 Compare February 17, 2023 18:40
cameel
cameel previously approved these changes Feb 17, 2023
ekpyron
ekpyron previously approved these changes Feb 20, 2023
Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Let's hope it actually works for the release :-).

@r0qs r0qs dismissed stale reviews from ekpyron and cameel via 930c8c0 February 20, 2023 13:28
@r0qs r0qs force-pushed the static-build-2004 branch from 6b564e0 to 930c8c0 Compare February 20, 2023 13:28
@cameel cameel merged commit 18b42dc into develop Feb 20, 2023
@cameel cameel deleted the static-build-2004 branch February 20, 2023 14:30
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.

Build static release binaries on ubuntu 20.04 again to avoid glibc incompatibilities.

4 participants