Node crate to folder #1632

lemunozm · 2023-11-29T09:39:13Z

Description

Move the node and its dependencies to a node folder

Changes and Descriptions

res src build.rs to node folder
Node using workspace dependencies
Some feature compilation fixes
Update scripts to now compile the node with cargo build -p centrifuge-chain
Modify scripts/tests.sh into scripts/cargo_split.sh to filter by feature and reuse compilation artifacts, improving the speed. It takes around 20 min to compile all crates for one feature in my M1.
Add subalfred target to ci/run-check.sh for a feature CI job checking this

Current issue (when moving the node) SOLVED

When you compile the workspace, the same crate dependency built is used for different crates of the workspace. If different crates use the same dependency with different features, the first version in the compiling process is used for both. This leads to feature problems, such as the compiler complaining about missing features when the feature is actually correctly set in the crate.

When compiling the node along the workspace crate, all crates use dependencies that at least contain the features they need (probably because its dependencies are compiled first). Nevertheless, when moving the node out, the compiling order changes and causes some crates to use some dependencies with some features missing.

A simple cargo check -F runtime-benchmarks fails because some crate uses a previous artifact that was built without runtime-benchmarks even when in its Cargo.toml, the feature is correctly set.

lemunozm

Relevant points

ci/run-check.sh

lemunozm · 2023-11-29T09:43:10Z

node/Cargo.toml

+[package.metadata.wasm-pack.profile.release]
+# `wasm-opt` has some problems on linux, see
+# https:/rustwasm/wasm-pack/issues/781 etc.
+wasm-opt = false


Does anyone know what was exactly the issue to check if it now works removing this? From the linked issue, it seems fixed, but I am not sure how to check it or evaluate it

lemunozm · 2023-11-29T09:44:34Z

pallets/bridge-mapping/Cargo.toml

@@ -1,30 +0,0 @@
-[package]


AFAIK, this module is deprecated and can be removed

lemunozm · 2023-11-29T10:00:29Z

Cargo.toml

@@ -66,6 +41,7 @@ members = [
 "pallets/rewards",
 "runtime/altair",
 "runtime/centrifuge",
+ "runtime/development",


I can not tell why, but you can compile development even if it is not on the list. Anyways, adding it here for correctness

ci/run-check.sh

docker/centrifuge-chain/Dockerfile

node/Cargo.toml

wischli

Overall LGTM but I think the root Cargo.toml was emptied too much 😅

node/Cargo.toml

pallets/liquidity-pools/Cargo.toml

wischli · 2023-12-12T10:37:54Z

scripts/cargo_split.sh

+#!/usr/bin/env bash
+
+# Usage
+# ./scripts/tests.sh check|test "-F=feature1,feature2,..." <--no-run>
+


The comment is off, should refer to cargo_split.sh instead of tests.sh

It was named previously so, but now you can execute any cargo <command>. That's why I changed the name to another more generic.

Thinking now that maybe even already exists such cargo tool 🤔

Cargo.toml

lemunozm · 2023-12-12T11:27:29Z

Seems the docker build command is failing. Could it be related to the last addition of #1646 ?

Tested and "it works on my machine"

lemunozm · 2023-12-13T08:51:46Z

As a test, I've reverted the already merged #1646, and now it works again.

I'm quite lost, any suggestion @gpmayorga @wischli

The CI job error is: https:/centrifuge/centrifuge-chain/actions/runs/7188961287/job/19579621674

wischli

There are a few outdated code paths which want to pull chain specs from the root /res not the moved /node/res:

./docker/docker-compose-local-chain.yml
./docker/docker-compose-local-relay.yml
./scripts/run_collator.sh

wischli · 2023-12-13T13:54:46Z

docker/centrifuge-chain/Dockerfile

@@ -20,8 +20,7 @@ FROM --platform=linux/amd64 docker.io/paritytech/ci-linux:production as builder
 WORKDIR /centrifuge-chain
 ARG FEATURES=""
 RUN FEATURES=$(echo ${FEATURES} | tr -d '"') \
- cargo build --locked --release --features=${FEATURES}
-
+ cargo build -p centrifuge-chain --locked --release --features=${FEATURES}


@lemunozm Can you try reverting the revert and removing the -p centrifuge-chain here? I have no clue how the pipeline change affects this PR, so this attempt is out of desperation.

Suggested change

cargo build -p centrifuge-chain --locked --release --features=${FEATURES}

cargo build --locked --release --features=${FEATURES}

I'll try!

BTW, Thanks for notifying the above files 🙌🏻

Just playing around with the above command, I'll try a cargo update. Maybe the --locked param is what is hitting us here

I tested everything and everything failed 🥲. Suuuper weird

Tested locally and also fails.

This seems to be the issue: https://stackoverflow.com/questions/74643818/docker-buildx-gives-exit-code-137-with-cargo

Tested locally and also fails.

This seems to be the issue: https://stackoverflow.com/questions/74643818/docker-buildx-gives-exit-code-137-with-cargo

IMO, the CI issue and Apple M1 docker build failures are unrelated. AFAIK, we cannot build our docker image locally on M1.

@lemunozm I deleted all cache related to this PR and re-ran the docker build manually in the actions and it succeeded 🎉 https:/centrifuge/centrifuge-chain/actions/runs/7206286656 Now re-scheduling the failed CI jobs here and hoping for deterministic behavior.

Uooo! Cool!

Then, I'm going to revert the cargo update done and adding again -p centrifuge-chain

wischli

Super happy CI is finally bending the knee. Let's get this merged ASAP!

lemunozm · 2023-12-14T15:04:28Z

Thanks so much for all your help on this last issue @wischli 🙏🏻

lemunozm · 2023-12-15T08:25:21Z

@mustermeiszer @NunoAlexandre I think we need an approval from any of you 🙏🏻

mustermeiszer

I disagree with moving the specs into the node folder. But no blocker.

lemunozm · 2023-12-15T09:13:44Z

Why not? Do they not belong to the node? Actually, those files are only referenced from the node crate.

mustermeiszer · 2023-12-15T10:17:44Z

Why not? Do they not belong to the node? Actually, those files are only referenced from the node crate.

The spec is general for our chain. It should be usable by any node in theory or by any project without forcing them to depend in the node itself.

lemunozm · 2023-12-15T10:19:27Z

Ok, that makes sense!

wischli · 2023-12-15T10:25:45Z

I disagree with moving the specs into the node folder. But no blocker.

I was thinking about this as well. I know at least two projects, which also moved the specs into the node directory and didn't raise this concern for that reason. But I definitely agree with out point that specs and the client are independent.

lemunozm · 2023-12-15T11:59:15Z

Thanks for sharing the research!

lemunozm · 2023-12-15T12:00:06Z

Maybe for now, we can keep it until we find the need to move this out (?)

lemunozm added 6 commits November 28, 2023 12:31

workspace dependecies for node in std

352dd18

minor fixes

c076005

node to folder

72677d0

remove unused bin tag

1a858e9

modify scripting commands

a7746ca

fixes and script changes

14e58b7

lemunozm added I3-annoyance The code behaves as expected, but "expected" is an issue. dependencies Pull requests that update a dependency file. I11-cleaning No mandatory issue that leave the repo more readable/organized labels Nov 29, 2023

lemunozm requested a review from gpmayorga November 29, 2023 09:39

lemunozm self-assigned this Nov 29, 2023

lemunozm commented Nov 29, 2023

View reviewed changes

lemunozm added 2 commits November 29, 2023 10:48

minor changes

d661070

add development to Cargo.toml

87d0539

lemunozm commented Nov 29, 2023

View reviewed changes

taplo fmt

22ddc05

lemunozm mentioned this pull request Nov 29, 2023

Move node crate to its own folder #1406

Closed

lemunozm added 2 commits December 1, 2023 09:38

revert shared features accross jobs

3571765

fix Cargo issues

4afe2c2

lemunozm added the Q5-hard Can be done by an experienced coder with a good knowledge of the codebase. label Dec 11, 2023

Merge remote-tracking branch 'origin/main' into node-to-crate

c67cabd

lemunozm marked this pull request as ready for review December 11, 2023 12:28

lemunozm requested review from mustermeiszer, NunoAlexandre, hieronx, wischli and cdamian as code owners December 11, 2023 12:28

fix docker script

f67ae73

cdamian reviewed Dec 12, 2023

View reviewed changes

ci/run-check.sh Show resolved Hide resolved

cdamian reviewed Dec 12, 2023

View reviewed changes

docker/centrifuge-chain/Dockerfile Show resolved Hide resolved

cdamian reviewed Dec 12, 2023

View reviewed changes

node/Cargo.toml Outdated Show resolved Hide resolved

wischli reviewed Dec 12, 2023

View reviewed changes

extra cleaning after comments

c5745b5

Merge remote-tracking branch 'origin/main' into node-to-crate

2bfc5a4

lemunozm force-pushed the node-to-crate branch from 8f7837a to 2bfc5a4 Compare December 12, 2023 19:14

Merge remote-tracking branch 'origin/main' into node-to-crate

f421f1d

wischli reviewed Dec 13, 2023

View reviewed changes

lemunozm force-pushed the node-to-crate branch from 01425e7 to f421f1d Compare December 13, 2023 15:37

fix resource path

3735e5d

lemunozm force-pushed the node-to-crate branch from 852089a to 3735e5d Compare December 14, 2023 13:36

Merge remote-tracking branch 'origin/main' into node-to-crate

ff5ea50

wischli approved these changes Dec 14, 2023

View reviewed changes

cdamian approved these changes Dec 14, 2023

View reviewed changes

lemunozm enabled auto-merge (squash) December 14, 2023 15:58

mustermeiszer approved these changes Dec 15, 2023

View reviewed changes

NunoAlexandre approved these changes Dec 15, 2023

View reviewed changes

lemunozm merged commit 1d1bdb7 into main Dec 15, 2023
11 checks passed

lemunozm deleted the node-to-crate branch December 15, 2023 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node crate to folder #1632

Node crate to folder #1632

lemunozm commented Nov 29, 2023 •

edited

Loading

lemunozm left a comment

lemunozm Nov 29, 2023

lemunozm Nov 29, 2023 •

edited

Loading

lemunozm Nov 29, 2023

wischli left a comment

wischli Dec 12, 2023 •

edited

Loading

lemunozm Dec 12, 2023

lemunozm commented Dec 12, 2023

lemunozm commented Dec 13, 2023 •

edited

Loading

wischli left a comment

wischli Dec 13, 2023

lemunozm Dec 13, 2023

lemunozm Dec 13, 2023

lemunozm Dec 14, 2023

lemunozm Dec 14, 2023

wischli Dec 14, 2023 •

edited

Loading

lemunozm Dec 14, 2023

lemunozm Dec 14, 2023

wischli left a comment

lemunozm commented Dec 14, 2023

lemunozm commented Dec 15, 2023

mustermeiszer left a comment

lemunozm commented Dec 15, 2023

mustermeiszer commented Dec 15, 2023

lemunozm commented Dec 15, 2023

wischli commented Dec 15, 2023

lemunozm commented Dec 15, 2023

lemunozm commented Dec 15, 2023

	cargo build -p centrifuge-chain --locked --release --features=${FEATURES}
	cargo build --locked --release --features=${FEATURES}

Node crate to folder #1632

Node crate to folder #1632

Conversation

lemunozm commented Nov 29, 2023 • edited Loading

Description

Changes and Descriptions

Current issue (when moving the node) SOLVED

lemunozm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

wischli Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Dec 12, 2023

lemunozm commented Dec 13, 2023 • edited Loading

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

lemunozm commented Dec 14, 2023

lemunozm commented Dec 15, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

lemunozm commented Dec 15, 2023

mustermeiszer commented Dec 15, 2023

lemunozm commented Dec 15, 2023

wischli commented Dec 15, 2023

lemunozm commented Dec 15, 2023

lemunozm commented Dec 15, 2023

lemunozm commented Nov 29, 2023 •

edited

Loading

lemunozm Nov 29, 2023 •

edited

Loading

wischli Dec 12, 2023 •

edited

Loading

lemunozm commented Dec 13, 2023 •

edited

Loading

wischli Dec 14, 2023 •

edited

Loading