Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: use nightly-2020-03-25 #427

Merged
merged 1 commit into from
Mar 25, 2020
Merged

Conversation

benma
Copy link
Collaborator

@benma benma commented Mar 25, 2020

This is to be able to use async/await syntax, which was merged a few
days ago into nightly for no_std:
rust-lang/rust#69033.

This feature will be in beta in April and stable in June, at which
point we can start to use those channels again.

rust-toolchain file is added as a sanity check or for devs/users not
using Docker.

@benma benma requested review from NickeZ and x1ddos March 25, 2020 09:44
@benma
Copy link
Collaborator Author

benma commented Mar 25, 2020

Dockerfile Outdated
@@ -132,9 +132,11 @@ ENV PATH /opt/lcov-1.14/bin:$PATH
# Install rust compiler
ENV PATH /opt/cargo/bin:$PATH
ENV RUSTUP_HOME=/opt/rustup
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | CARGO_HOME=/opt/cargo sh -s -- --default-toolchain 1.42.0 -y
# Also update ./rust-toolchain when changing the default toolchain.
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | CARGO_HOME=/opt/cargo sh -s -- --default-toolchain nightly-2020-03-25 -y
Copy link
Contributor

Choose a reason for hiding this comment

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

The rust-toolchain is very helpful! Could we do something like this?

COPY rust-toolchain /tmp/
RUN ... | CARGO_HOME=/opt/cargo sh -s -- --default-toolchain $(cat /tmp/rust-toolchain) -y

So to attempt to keep them in sync.

Copy link
Collaborator Author

@benma benma Mar 25, 2020

Choose a reason for hiding this comment

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

Can be fragile, e.g. if someone slips in a newline in there, or deletes the file, it will break the docker build without anyone noticing

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Someone could also update Dockerfile while forgetting to update rust-toolchain or the other way around. :)
Either way it fine with me!

@NickeZ
Copy link
Collaborator

NickeZ commented Mar 25, 2020

With this change we technically don't need the RUSTC_BOOTSTRAP=1 any more. Any thoughts on removing that?

@benma
Copy link
Collaborator Author

benma commented Mar 25, 2020

With this change we technically don't need the RUSTC_BOOTSTRAP=1 any more. Any thoughts on removing that?

I'd still keep it with the hope of going beta->stable in the next months. Let's see if we can resist the temptation of nightly :D

Copy link
Contributor

@x1ddos x1ddos left a comment

Choose a reason for hiding this comment

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

LGTM

@NickeZ
Copy link
Collaborator

NickeZ commented Mar 25, 2020

I would prefer that you cat the rust-toolchain file before approving this PR as @x1ddos wrote.

@benma
Copy link
Collaborator Author

benma commented Mar 25, 2020

I would prefer that you cat the rust-toolchain file before approving this PR as @x1ddos wrote.

I'm not for it:

Can be fragile, e.g. if someone slips in a newline in there, or deletes the file, it will break the docker build without anyone noticing

@benma
Copy link
Collaborator Author

benma commented Mar 25, 2020

Instead of rust-toolchain, i could add an ENV var to the dockerfile and use it in RUSTUP_TOOLCHAIN? https://github.com/rust-lang/rustup#override-precedence

Downside: works only with CMakeLists, not when running cargo test directly.

@x1ddos
Copy link
Contributor

x1ddos commented Mar 25, 2020

Can be fragile, e.g. if someone slips in a newline in there, or deletes the file, it will break the docker build without anyone noticing

TBH that's what code reviews and CIs are for :)

If RUSTUP_TOOLCHAIN works as an alternative, why not. I'd imagine something like:

docker build ... -e RUSTUP_TOOLCHAIN=$(cat rust-toolchain)

but it's equivalent to the other solution anyway.

@x1ddos
Copy link
Contributor

x1ddos commented Mar 25, 2020

Ah sorry, forget about docker build -e.... I don't know what I was thinking.

@x1ddos
Copy link
Contributor

x1ddos commented Mar 25, 2020

Can be fragile, e.g. if someone slips in a newline in there, or deletes the file, it will break the docker build without anyone noticing

TBH that's what code reviews and CIs are for :)

Plus, if someone submits a change to update rust-toolchain, I'd expect them to also run with it manually to verify it works.

@NickeZ
Copy link
Collaborator

NickeZ commented Mar 25, 2020

Solve it any way you want, this string: nightly-2020-03-25 should only be in one place in the project.

This is to be able to use async/await syntax, which was merged a few
days ago into nightly for no_std:
rust-lang/rust#69033.

This feature will be in beta in April and stable in June, at which
point we can start to use those channels again.

rust-toolchain file is added as a sanity check or for devs/users not
using Docker.
@benma
Copy link
Collaborator Author

benma commented Mar 25, 2020

Ok did the cat thing

@benma benma merged commit 0b15e7d into BitBoxSwiss:master Mar 25, 2020
@benma benma deleted the rust-nightly branch March 25, 2020 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants