solidity
solidity copied to clipboard
CI config tweaks after CircleCI review
~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:
- I think that
b_win
job is superfluous given that we haveb_win_release
as well. They pretty much always fail or succeed together and for some reasonb_win
is actually sometimes not showing all compiler errors in its output andb_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 likeb_win_release
so I also renamed it tob_osx_release
.
- While at it I noticed that
- CircleCI has deprecated some of the images we're using (see Legacy Convenience Image Deprecation) so I'm switching to alternatives:
-
cimg/node
insteadcircleci/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 ofcircleci/buildpack-deps
. In some cases we were using specificallycircleci/buildpack-deps:focal
and this tag is not available forcimg/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 likeinput_sol.sol
vsin_sol.sol
and makes the comparison fail. To fix this I explicitly set locale toC
whenever we use these scripts.
- 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
-
There are two more things that require more work and are not urgent so I only created issues for them:
- #13057
- #13056
Looks like I also need to set the locale on Windows somehow...
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?
I rebased and made it worse :muscle:
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?
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.
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?
The primary motivation for dropping it was that it was one of the recommendations from CircleCI's config review:
The
b_win
andb_win_release
jobs are largely duplicated, only with an additional environment variable included in the latter. The same occurs for thet_win
andt_win_soltest
jobs. Consider optimising this further, by merging these jobs into a more efficient flow, and also perhaps including awhen
clause to run only one workflow depending on whether theFORCE_RELEASE:
environment variable is set or not, or by only running one of these on themain
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.