[CI] Update dockerfiles that build-wasm to include bitcoinsuite-chronik-client
ClosedPublic
Actions

Authored by bytesofman on Sun, Dec 22, 11:57.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC99031b560307: [CI] Update dockerfiles that build-wasm to include bitcoinsuite-chronik-client

Summary

See latest Cashtab CI failure

With a new module in the monorepo that is part of the top level cargo.toml scope, all CI apps that need this scope are broken unless they include everything in this workspace.

Since this has happened twice now in less than a month ... we should have some better way of catching this. However, the issue is known and the fix is simple enough. Getting CI to build all dockerfiles every time cargo.toml changes, mb?

Adding new stuff to the top level cargo.toml is not expected to be this common in the future, so the bigger risk is this doesn't happen again for 18 months and then when it does, we don't remember what's going on.

Test Plan

grep "FROM rust:1.76.0 AS wasmbuilder" . and confirm all dockerfiles that build from rust have this added in the same way it was added for the explorer

Diff Detail

Repository

rABC Bitcoin ABC

Branch

workspace-mod-ci-fix-again

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 31739
Build 62972: Build Diff
Build 62971: arc lint + arc unit

Event Timeline

bytesofman created this revision.Sun, Dec 22, 11:57

Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Dec 22, 11:57

Harbormaster completed remote builds in B31739: Diff 51713.Sun, Dec 22, 11:57

bytesofman published this revision for review.Sun, Dec 22, 11:58

Maybe we shouldn't use workspace for this?

This revision is now accepted and ready to land.Sun, Dec 22, 20:33

In D17384#395322, @Fabien wrote:

Maybe we shouldn't use workspace for this?

mb not but more possible semantic confusions here

in this case, I am just using "workspace" because this is in the error logs

#30 0.570 error: failed to load manifest for workspace member `/app/modules/bitcoinsuite-chronik-client`

So, seems to be a rust / Cargo.toml thing that I am not familiar with. But basically means everything mentioned in the top level cargo.toml needs to be available in a docker container that is referencing that cargo.toml. Rust optimization question as to whether we should include all the rust apps in this top level file, I haven't looked into this.

Confusing because "workspace" is also an npm term. At the moment, we are not using npm workspaces anywhere in the monorepo (though we have looked at it before and may implement at some point in the future).

So -- this diff does continue the pattern of "docker files only build the minimum of what they need." Ofc, dockerfiles for stuff like cashtab really do not need the explorer or bitcoinsuite-chronik-client. They must include them becauase of this rust cargo.toml req.

Doesn't seem like a big problem from my perspective as long as there are some rust related benefits of keeping this unified top level cargo.toml. Tho it does take this kind of periodic manual awareness / patching. Also, mb it is bad practice for us to be throwing all our rust apps in this top level cargo.toml. Though seems like a good idea if we are trying to make sure everything is using the same rust versions and rust deps.

Closed by commit rABC99031b560307: [CI] Update dockerfiles that build-wasm to include bitcoinsuite-chronik-client (authored by bytesofman). · Explain WhySun, Dec 22, 21:07

This revision was automatically updated to reflect the committed changes.

bytesofman added a commit: rABC99031b560307: [CI] Update dockerfiles that build-wasm to include bitcoinsuite-chronik-client.

confirmed this did patch the Cashtab CI issue