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

Update rve toolchain in unified image #615

Merged
merged 6 commits into from
Dec 12, 2023
Merged

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Dec 11, 2023

Update the unified-ci script with the latest version of the risc-v toolchain and openssl to version 3.1.4

cc https://github.com/paritytech/ci_cd/issues/910

@pgherveou pgherveou marked this pull request as ready for review December 11, 2023 15:18
@@ -281,6 +275,18 @@ RUN cargo install --git https://github.com/paritytech/ink-waterfall.git csv-comp
# parity-scale-codec
RUN cargo +nightly-${RUST_NIGHTLY_VERSION} install grcov rust-covfix xargo dylint-link

# Install rve-nightly-2023-04-05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

@alvicsam alvicsam requested review from chevdor, EgorPopelyaev and a team December 12, 2023 11:01
Copy link
Contributor

@chevdor chevdor left a comment

Choose a reason for hiding this comment

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

LGTM besides the small improvements for the Container image

Comment on lines +111 to 115
RUN cargo install cargo-web wasm-pack cargo-deny cargo-hack \
mdbook mdbook-mermaid mdbook-linkcheck mdbook-graphviz mdbook-admonish mdbook-last-changed

RUN cargo install cargo-nextest --locked
RUN cargo install cargo-spellcheck cargo-nextest --locked

Copy link
Contributor

Choose a reason for hiding this comment

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

It is good practise to avoid the multipliction of layers in a docker image.
So instead of:

RUN cargo install foo
RUN cargo install bar

the following is preferable:

RUN cargo install foo bar

If you prefer a mutiline option to keep things clear, you may use:

RUN cargo install \
            foo bar \
            baz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just changed what was breaking, I think we should move all these install to use --locked.
Otherwise this could break for future version as I discovered here.


RUN wget https://github.com/paritytech/rustc-rv32e-toolchain/releases/download/rust-rve-nightly-2023-04-05-x86_64-unknown-linux-gnu/rust-rve-nightly-2023-04-05-x86_64-unknown-linux-gnu.tar.zst && \
tar --zstd -xf rust-rve-nightly-2023-04-05-x86_64-unknown-linux-gnu.tar.zst && \
mv tmp/rve-nightly /usr/local/rustup/toolchains && \
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be safer to use /tmp instead of tmp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's ./tmp here that is extracted from the tarball

mdbook mdbook-mermaid mdbook-linkcheck mdbook-graphviz mdbook-admonish mdbook-last-changed

RUN cargo install cargo-nextest --locked
RUN cargo install cargo-spellcheck cargo-nextest --locked

# generic ci | diener 0.4.6 | NOTE: before upgrading please test new version with companion build, example can be found here: https://github.com/paritytech/substrate/pull/12710
RUN cargo install diener --version 0.4.6
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be good to pack all the cargo install together if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why we don't do --locked for all of them. I assumed it was to save some downloads and re-use as many common crates as possible

@alvicsam
Copy link
Contributor

@chevdor is it okay that openssl will be upgraded in ci image to 3.1.4 version?

@chevdor
Copy link
Contributor

chevdor commented Dec 12, 2023

@alvicsam I don't know which version is in the image right now, assuming the new version does not involve breaking changes, it will be fine. I am not aware of anything depending on a specific version.

We will see that in CI way before we need to use it in the release anyway. So it is safe and wise to try updating to a current version.

@alvicsam
Copy link
Contributor

@chevdor It is needed for adding risc-v toolchain to the ci needed. It requires a newer openssl version. Current version is 1.1.sth and with this PR openssl will be installed from unstable and updated to 3.1.4 version. The newer openssl version was tested in this PR and afaiu works well.

@alvicsam alvicsam merged commit f10cf95 into master Dec 12, 2023
4 checks passed
@alvicsam alvicsam deleted the pg/update-riscv-unified-image branch December 12, 2023 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants