Allow builds without Docker inside of Docker for cbrotli-wasm #2748
Conversation
|
We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2 |
nhovsmith-cb
changed the title
|
@cla-bot check |
cla-bot
bot
added
the
s
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Thank you for the PR. However, this is built in Docker because it otherwise complicates the needed build environment to build nitro. In particular, I think it's tricky to setup the native WASM cbrotli build for MacOS if I recall correctly. Therefore, we can't accept this PR in its current form, but perhaps you could make it an optional variable defaulting to building with Docker |
Makefile
Outdated
| test -f target/lib-wasm/libbrotlienc-static.a || ./scripts/build-brotli.sh -w | ||
| test -f target/lib-wasm/libbrotlidec-static.a || ./scripts/build-brotli.sh -w | ||
| else | ||
| test -f target/lib-wasm/libbrotlicommon-static.a || ./scripts/build-brotli.sh -w -d |
There was a problem hiding this comment.
I'd try setting a variable to -d or empty and use that here in order to avoid duplicating all of these
Makefile
Outdated
| test -f target/lib-wasm/libbrotlienc-static.a || ./scripts/build-brotli.sh -w -d | ||
| test -f target/lib-wasm/libbrotlidec-static.a || ./scripts/build-brotli.sh -w -d | ||
| endif | ||
| test -f target/lib-wasm/libbrotlicommon-static.a || ./scripts/build-brotli.sh $(ARGS) |
There was a problem hiding this comment.
Almost LGTM but ARGS is way too generic for this, could we call it CBROTLI_WASM_BUILD_ARGS ?
There was a problem hiding this comment.
Also, the default for outside of docker needs to be using docker to build this. The default inside docker can be not using docker-in-docker, that's fine.
nhovsmith-cb
changed the title
|
@PlasmaPower I see that a handful or required actions are failing. GO Tests seem to be failing on arbitrator/langs/bf/target/debug/stylus-sdk-bf and the Arbitrator CI tests are failing because of no patch-package I am not familiar with either of these aspects of the arbitrum. Have you seen this before? Is there a simple/obvious thing I am in order to get the checks to pass and merge the PR? |
|
The issue seems to be here https://github.com/OffchainLabs/nitro/actions/runs/11468406141/job/31915947068?pr=2748#step:17:23 |
|
I'm not sure your syntax is correct. Perhaps the Makefile variable needs to be declared outside the rules for building a specific target. That'd be my guess. |
Head branch was pushed to by a user without write access
Makefile
Outdated
| test -f target/lib-wasm/libbrotlidec-static.a || ./scripts/build-brotli.sh -w -d | ||
| test -f target/lib-wasm/libbrotlicommon-static.a || ./scripts/build-brotli.sh $(CBROTLI_WASM_BUILD_ARGS) | ||
| test -f target/lib-wasm/libbrotlienc-static.a || ./scripts/build-brotli.sh $(CBROTLI_WASM_BUILD_ARGS) | ||
| test -f target/lib-wasm/libbrotlidec-static.a || ./scripts/build-brotli.sh $(CBROTLI_WASM_BUILD_ARGS) |
There was a problem hiding this comment.
One other thing, these should always be built as wasm, so could we have -w outside the CBROTLI_WASM_BUILD_ARGS? I.e. run ./scripts/build-brotli.sh -w $(CBROTLI_WASM_BUILD_ARGS) and have CBROTLI_WASM_BUILD_ARGS default to just -d
Give the option to build natively as opposed to docker when calling
.make/cbrotli-wasm: