solidity icon indicating copy to clipboard operation
solidity copied to clipboard

CI config tweaks after CircleCI review

Open cameel opened this issue 2 years ago • 1 comments

~Depends on #13063.~ Merged.

This is a bunch of CI tweaks addressing some of the feeback we received in CircleCI's config review. Here's an overview of what I'm changing:

  1. I think that b_win job is superfluous given that we have b_win_release as well. They pretty much always fail or succeed together and for some reason b_win is actually sometimes not showing all compiler errors in its output and b_win_release does not have this problem. We're doing just fine with one job for macOS so I think we can do with one for Windows. macOS and Windows jobs are also the ones we're paying the most for so removing this is is going to be a big saving.
    • While at it I noticed that b_osx is actually running a release config, just like b_win_release so I also renamed it to b_osx_release.
  2. CircleCI has deprecated some of the images we're using (see Legacy Convenience Image Deprecation) so I'm switching to alternatives:
    • cimg/node instead circleci/node. The downside here is that the tags for major node.js versions like 12, 14, 16, etc. are no longer available and it doesn't looks like they'll come back anytime soon (https://github.com/CircleCI-Public/cimg-node/issues/130). For now I had to specify it down to patch version. And we'll have to remember about keeping them up to date now.
    • cimg/base instead of circleci/buildpack-deps. In some cases we were using specifically circleci/buildpack-deps:focal and this tag is not available for cimg/base but this was for jobs like pylint or grammar so I don't think this matters.
      • This actually revealed a small problem with our bytecode report scripts. Looks like the new image has a different default locale, which affects the sort order for Bash globs and for Python's sorted(). This affects cases like input_sol.sol vs in_sol.sol and makes the comparison fail. To fix this I explicitly set locale to C whenever we use these scripts.

There are two more things that require more work and are not urgent so I only created issues for them:

  • #13057
  • #13056

cameel avatar May 25 '22 19:05 cameel

Looks like I also need to set the locale on Windows somehow...

cameel avatar May 25 '22 21:05 cameel

PR changes look fine and reflect what I can find in the description. In regard to the node version, can we maybe have a script that provides warning whenever a new release is available?

wechman avatar Sep 06 '22 09:09 wechman

I rebased and made it worse :muscle:

nikola-matic avatar Oct 07 '22 11:10 nikola-matic

This is now ready. All jobs pass except for the hardhat one, which is broken for unrelated reasons. Note that there are a few jobs that look like they're still running but that's because the were renamed/removed in the PR while they are still marked as required on github. Once the PR is approved, I'll adjust repo config to fix this, just before this is merged.

One important point to consider when reviewing this: are we fine with dropping one of the windows builds?

cameel avatar Oct 26 '22 09:10 cameel

I see bytecodecompare has different compiler versions, which I guess is the source of failure (one of, haven't checked in detail whether there's anything else).

It was failing because I removed one of the windows build jobs and switched the bytecode comparison it to use the other - which was running with FORCE_RELEASE=ON. That flag makes it identify as a release, which affects the version string - so version looked different than on other platforms. I fixed that by switching to removing the other windows job.

cameel avatar Oct 26 '22 10:10 cameel

This is now ready. All jobs pass except for the hardhat one, which is broken for unrelated reasons. Note that there are a few jobs that look like they're still running but that's because the were renamed/removed in the PR while they are still marked as required on github. Once the PR is approved, I'll adjust repo config to fix this, just before this is merged.

One important point to consider when reviewing this: are we fine with dropping one of the windows builds?

Can you explain in more detail why you dropped b_win_release (I saw the description about version difference, but still don't understand)? Also, what does FORCE_RELEASE do if the builds are already in release mode?

nikola-matic avatar Oct 26 '22 10:10 nikola-matic

The primary motivation for dropping it was that it was one of the recommendations from CircleCI's config review:

The b_win and b_win_release jobs are largely duplicated, only with an additional environment variable included in the latter. The same occurs for the t_win and t_win_soltest jobs. Consider optimising this further, by merging these jobs into a more efficient flow, and also perhaps including a when clause to run only one workflow depending on whether the FORCE_RELEASE: environment variable is set or not, or by only running one of these on the main branch.

Windows and macOS jobs are pretty expensive and the difference between b_win and b_win_release is very small. At some point we considered moving one of them to the nightly run but really, but I'm not sure if such a build gives us all that much if it's not running on every PR.

I don't really remember any case where one of them failed and the other did not. At most, I noticed on multiple occasions that the build log from b_win_release is missing some error messages while they do show up in b_win. Not sure why it happens and we should just fix it but so far we never had much motivation to spend time on it.

Also, what does FORCE_RELEASE do if the builds are already in release mode?

When we build a release we include an empty prerelease.txt file in repo root. Setting FORCE_RELEASE=ON does the same thing. Based on that our CMake config sets a different version string (it no longer says it's a development build) and the C++ code has access to that via a preprocessor macro. In this mode the compiler also no longer emits a warning that it's a pre-release version. There are probably some minor things it can affect in the build, but I'd have to look for them to tell. One you might be able to notice is if you break stripping of the pre-release warning and CI starts failing for only one of these builds. Other differences are unlikely to be very noticeable, it's just that it's too late to fix them when we're doing an actual release. We want to catch them earlier.

We still have a Linux build in this mode and it might be enough. For some reason we never had a macOS or emscripten build like this.

cameel avatar Oct 26 '22 10:10 cameel