Docker ci fix

[dtebbs]

No description provided.

[dtebbs]

This seems to be fixed by adding a fallback to build from source if the binary install fails, but it takes approx 1h20m to complete. This may be OK for the purposes of this release, but should probably be addressed at some stage.

The options I can see are:

• create an alpine-friendly binary version of solc somewhere, and add some logic to install that into the container before the python env is set up
• change the distro for the client container (which, under the current image dependencies, means changing it for all images)
• drop the client docker image
• EDIT: pull an alpine-friendly binary from somewhere, e.g. https://hub.docker.com/layers/ethereum/solc/0.8.0-alpine/images/sha256-3bd355f2b9429c5ded37ddfe8e596da947535ffbb86aede9b953cb9ad48a2c53?context=explore

[dtebbs]

@AntoineRondelet the last commit updates Dockerfile-client to pull the binary from ethereum/solc:0.8.1-alpiine, which shortens the container build time. The disadvantage is that we have the compiler version number hard-coded in the Dockerfile. I'd suggest we use one of these options (the long source build or copying the binary), and create an issue to come back to this in the future. I would probably lean towards the binary, but don't have a strong preference.

(Once the current build passes I'll make this PR target develop. For now it targets master in order to force the docker builds.)

[AntoineRondelet]

* EDIT: pull an alpine-friendly binary from somewhere, e.g. https://hub.docker.com/layers/ethereum/solc/0.8.0-alpine/images/sha256-3bd355f2b9429c5ded37ddfe8e596da947535ffbb86aede9b953cb9ad48a2c53?context=explore

Good idea @dtebbs - that would prevent major changes to the images before the release. Alternatively, building from source is also fine (the images are only built when we PR master) so 1h or so of build time isn't too much a problem. That being said, if we can build on top of an already existing layer we should definitely do it.

[AntoineRondelet]

@AntoineRondelet the last commit updates Dockerfile-client to pull the binary from ethereum/solc:0.8.1-alpiine, which shortens the container build time. The disadvantage is that we have the compiler version number hard-coded in the Dockerfile. I'd suggest we use one of these options (the long source build or copying the binary), and create an issue to come back to this in the future. I would probably lean towards the binary, but don't have a strong preference.

(Once the current build passes I'll make this PR target develop. For now it targets master in order to force the docker builds.)

Ah! just saw this message as I was writing mine above..
Yes, sounds good, let's use the binary from the alpine-solc layer for now if that works. I'm not too worried about building from source - as per my comment above - but if we can make the whole process faster by using an already existing binary, let's do that for now, and open a ticket to keep this on our radar for the next release.
Re: the compiler version being hardcoded -> yes that's somehow suboptimal as we'd like to keep the places where these values are hardcoded condensed (and avoid hardcoding it everywhere which is error prone and not elegant) - however, the compiler version is already (somehow) hardcoded in the pragmas (at least the minor, not the patches...) of the contracts and in the python client code, so having it in this Dockerfile is simply not elegant, but doesn't pose any major issue (the code of the release targets the 0.8 solc version anyway). So if that all works, let's move forward with this approach.

[dtebbs]

I've created #373 to track this. As you say, let's move forward with this.

however, the compiler version is already (somehow) hardcoded in the pragmas (at least the minor, not the patches...) of the contracts and in the python client code, so having it in this Dockerfile is simply not elegant, but doesn't pose any major issue (the code of the release targets the 0.8 solc version anyway).

Just to clarify, the problematic part of hardcoding the compiler version here is that it has to be kept the same as in constants.py (up until now, constants.py was the single place this was specified). If they don't match, the alpine binary will be silently ignored and the version from constants.py will be installed from source, with no error in the CI.

[AntoineRondelet]

the problematic part of hardcoding the compiler version here is that it has to be kept the same as in constants.py (up until now, constants.py was the single place this was specified).

Yes that's what I meant by:

that's somehow suboptimal as we'd like to keep the places where these values are hardcoded condensed (and avoid hardcoding it everywhere which is error prone and not elegant)

Needing to hardcode the same value all over the place is error prone since one has great chances to forget to change the value somewhere and trigger some potentially unexpected behaviors or plain errors (compilation or runtime - depending on the context).

If they don't match, the alpine binary will be silently ignored and the version from constants.py will be installed from source, with no error in the CI.

Yes, now that you added the try/except that's trigger the installation in except indeed (I count that as an "unexpected behavior" due to inconsistent hardcoded values - as per my point above - because if one bothers to move a specific binary version in the container it must be for a reason - i.e. to avoid to build from source, yet the build happens if the 2 values do not match)

[AntoineRondelet]

LGTM, thanks for fixing this one @dtebbs