onflow / flow-emulator

The Flow Emulator is a lightweight tool that emulates the behaviour of the real Flow network
https://onflow.org
Apache License 2.0
84 stars 49 forks source link

Docker builds are broken #534

Closed turbolent closed 8 months ago

turbolent commented 10 months ago

Problem

CI automatically builds Docker images. They're currently broken, see e.g. https://github.com/onflow/flow-emulator/actions/runs/7212048234/job/19649026562#step:7:648

The build errors show problems in the crypto library.

Steps to Reproduce

See link above / "Docker Image" CI job's logs.

Acceptance Criteria

Docker images are built correctly.

tarakby commented 10 months ago

@sideninja and I faced these problems yesterday with the CLI build as well. First we need to enable cgo with CGO_ENABLED=1, it's also preferred to add the flag CGO_CFLAGS="-O -D__BLST_PORTABLE__" in case the building or target machines are old (this should be added to the Dockerfile), but then the main problem becomes cross-compilation. I can see the repo targets both x86 and aarch64 . Not sure if one of the two builds had succeeded from the logs. I guess it also depends on the architecture of the github action machine. But anyway the Dockerfile needs to be updated to handle cross-compilation properly, since CGO is now being used.

I was planning to work on these tasks with the new crypto library repo by repo, but things went fast with the EVM releases. A quick temp fix that worked for CLI is to go back to the previous crypto library onflow/flow-go/crypto@v.0.24.9 in a replace statement.

turbolent commented 10 months ago

This also breaks building the Cadence language server to WebAssembly. Before it was possible to build without CGo, how come we need it now?

tarakby commented 10 months ago

This also breaks building the Cadence language server to WebAssembly

Does the Cadence language server depend on the emulator?

how come we need it now?

In the past there was a build tag that is needed if BLS stuff need to be compiled. CLI and emulator always avoided that build tag and excluded BLS (mainly to avoid an extra setup step needed for BLS, not to avoid cgo). The new crypto module doesn't require the setup build, so the build tag was removed.

Can you please point me to the build script of the Cadence language server?

bluesign commented 10 months ago

does emulator need BLS? I don't know if evm stuff needs it, but if not maybe we can use build tag again.

tarakby commented 9 months ago

Regardless of EVM, Cadence natively supports BLS, so it makes sense for the emulator to support it too. It's been disabled on the emulator all this time. (CLI however does not need BLS and can disable it).

In general, I would like to move away from the build tags within the crypto library as they have been causing development friction. The next steps can be:

sideninja commented 9 months ago

@tarakby Would moving to /bls package be hard for you? We can try and setup builds on actual OSs so we avoid cross-compiling, but if the bls sub-package is not a lot of work it kind of makes sense and it will also avoid similar issues for other possible tools.

tarakby commented 9 months ago

You are right that the crypto library should be splitting sub-packages. I've tried it a while ago but noticed it requires larger refactors than I thought. It's not a quick fix unfortunately. Moreover it only solves the build issue for CLI (because it doesn't require BLS), but won't solve the build issue of emulator (requires BLS).

tarakby commented 9 months ago

I am fixing the build in https://github.com/onflow/flow-emulator/pull/548 and I was wondering how I can make sure the built images can run correctly. Is there a test you usually perform for this?

sideninja commented 9 months ago

I am fixing the build in #548 and I was wondering how I can make sure the built images can run correctly. Is there a test you usually perform for this?

That's always painful. One way to test is to use act https://github.com/nektos/act but for architecture critical tests it might be a problem since the env is not the same, the second way is to fork the repo and test there. The third way is to create a temporary tag that is something like v0-test and then make sure they are deleted.