Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Require --locked when building runtime #2813

Merged
merged 6 commits into from
Jun 6, 2019

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jun 6, 2019

The CI runs the various build.sh files that are touched here.

If someone performs changes that require an update of the Cargo.lock, this PR will force them to commit it.

@tomaka tomaka added the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Jun 6, 2019
@tomaka tomaka requested review from bkchr and TriplEight June 6, 2019 10:14
@tomaka
Copy link
Contributor Author

tomaka commented Jun 6, 2019

The drawback of this (that I didn't think about) is that people who do a change that requires modifying the Cargo.lock, they will have to manually run cargo build or cargo update in the appropriate directories instead of using the provided script.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be configurable. You could add some env variable.

Otherwise the people will complain that they can not build the wasm files locally and need to switch to the directory to run cargo update.

@tomaka
Copy link
Contributor Author

tomaka commented Jun 6, 2019

You could add some env variable.

@TriplEight What can I do here? Is there an environment variable that tells us whether we are in the CI?

@TriplEight
Copy link
Contributor

there is also scripts/update.sh which is almost the same as scripts/build.sh but with cargo update

@TriplEight
Copy link
Contributor

Actually I wanted to refactor all this process of building the wasm binaries, and I think I'll need @gabreal help.

@@ -6,7 +6,7 @@ if cargo --version | grep -q "nightly"; then
else
CARGO_CMD="cargo +nightly"
fi
CARGO_INCREMENTAL=0 RUSTFLAGS="-C link-arg=--export-table" $CARGO_CMD build --target=wasm32-unknown-unknown --release
CARGO_INCREMENTAL=0 RUSTFLAGS="-C link-arg=--export-table" $CARGO_CMD build --target=wasm32-unknown-unknown --release $CARGO_BUILD_EXTRA_OPTIONS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't tested but wouldn't you need to append $@ instead of $CARGO_BUILD_EXTRA_OPTIONS when you write further down in ./scripts/build.sh:

./build.sh $CARGO_BUILD_EXTRA_OPTIONS

otherwise ^^ one is unneded if the env variable should be taken directly from the .gitlab-ci.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's working, so I don't know.

scripts/build.sh Outdated
@@ -19,7 +19,7 @@ do
echo "*** Building wasm binaries in $SRC"
cd "$PROJECT_ROOT/$SRC"

./build.sh
./build.sh $CARGO_BUILD_EXTRA_OPTIONS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required.

@tomaka
Copy link
Contributor Author

tomaka commented Jun 6, 2019

Seems to work as well after @gabreal's changes.

@bkchr bkchr merged commit d6e91c1 into paritytech:master Jun 6, 2019
@tomaka tomaka deleted the locked-scripts branch June 6, 2019 16:30
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
* Require --locked when building runtime

* Update locks

* Do it in a different way

* Accidentally reverted Cargo.lock

* pass on arguments in build.sh scripts
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants