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

Add rust-toolchain file #154

Merged
merged 1 commit into from
Jul 20, 2020
Merged

Conversation

shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Jul 17, 2020

Description

This instructs cargo and rustc to use the proper toolchain version, as
per https://github.com/rust-lang/rustup#the-toolchain-file, we can make
this more specific (following
https://github.com/rust-lang/rustup#toolchain-specification) if that
turns out useful.


For contributor use:

  • Unit tests written
  • Added test to CI if applicable
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

This instructs cargo and rustc to use the proper toolchain version, as
per https://github.com/rust-lang/rustup#the-toolchain-file, we can make
this more specific (following
https://github.com/rust-lang/rustup#toolchain-specification) if that
turns out useful.
@adizere
Copy link
Member

adizere commented Jul 20, 2020

Seems to be working!

adi@styx:ibc-rs $ rustup show
Default host: x86_64-apple-darwin
rustup home:  /Users/adi/.rustup

stable-x86_64-apple-darwin (overridden by '/Users/adi/Hammers/ibc-rs/rust-toolchain')
rustc 1.45.0 (5c1f21c3b 2020-07-13)

However, to help me understand better what exactly are we doing with this PR, two remarks:

  1. Is there anything we could prevent from breaking if we manually override toolchain to use stable? For instance, would we prevent issue test-nightly-coverage failures #135 from happening? I suspect this override has no effect on CI but I'm not 100%.
  2. Can we please open an issue and motivate the PR (maybe with answers to the above question)?

Thanks!

@shonfeder
Copy link
Contributor Author

shonfeder commented Jul 20, 2020

Sorry for not providing enough context and motivation here. btw, I should note that I'm not %100 sure we want this, but it seemed useful to me.

Motivation

In general, I believe it is best practice to strive for fully configured dev environments and fully reproducible builds. This improves the security and reliability of software deployment and the velocity of development. The latter is mainly achieved due to reduced cognitive overhead from having to track the system environment and dependency chain in one's head, and reduced friction from "but it works on my system" failures.

It was my understanding that this project is being developed on stable, but a user (such as myself) might have their system configured to use nightly as the default toolchain. The addition of the toolchain file here is to automate switching to the correct toolchain. As per the first link my PR description:

Some projects though find themselves 'pinned' to a specific release of Rust and want this information reflected in their source repository. This is most often the case for nightly-only software that pins to a revision from the release archives.

In these cases the toolchain can be named in the project's directory in a file called rust-toolchain

I would contend that we can and should take advantage of this pinning, even though we are not pinning to nightly! We have a lower bound on rustc stated in the readme, and clear evidence that builds are not reliable on nightly, so I think it would be wise to pin the devenv to a version of the toolchain that is considered standard.

This is also follows ANSII-FR's (National Cybersecurity Agency of France) guide to secure rust development:

Development of a secure application must be done using a fully stable toolchain, for limiting potential compiler, runtime or tool bugs.
(https://anssi-fr.github.io/rust-guide/02_devenv.html#rule-a-iddenv-stableadenv-stable)

If we are not specifying the toolchain in the dev env, then we are not doing anything to enforce development on stable, since developers are just left to use whatever happens to work on their machine.

Addressing questions

  1. Is there anything we could prevent from breaking if we manually override toolchain to use stable? For instance, would we prevent issue test-nightly-coverage failures #135 from happening? I suspect this override has no effect on CI but I'm not 100%.

Not if we keep the correct CI configuration. Currently every step explicitly specifies whether it should be run in stable or nightly, and according to the rust-toolchain action's documentation and the evident failure of the nightly check on this PR, that overrides the setting in this file.

Does this force stable in every case?

No. Using the ANSII-FR recommended practice, devs can easily opt-in to running things on nightly (or some more specific toolchain version), when needed. E.g.:

[sf@comp ibc-rs]$ cargo --version
cargo 1.44.1 (88ba85757 2020-06-11)
[sf@comp ibc-rs]$ cargo +nightly --version
cargo 1.46.0-nightly (4f74d9b2a 2020-07-08)

@shonfeder shonfeder requested a review from adizere July 20, 2020 19:04
@greg-szabo
Copy link
Member

Wow, very well researched! I guess it's a nice warning/prevention for users with sloppy settings.

I just set my cargo default to nightly this morning and indeed cargo test succeeded form this branch.

I think it's going to be slightly confusing for nightly users but we don't want to implement nightly features as far as I understand.

@greg-szabo greg-szabo assigned greg-szabo and unassigned greg-szabo Jul 20, 2020
@greg-szabo greg-szabo self-requested a review July 20, 2020 20:21
@greg-szabo greg-szabo merged commit 96ee973 into master Jul 20, 2020
@greg-szabo greg-szabo deleted the shon/add-rustup-toolchain-file branch July 20, 2020 20:22
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
This instructs cargo and rustc to use the proper toolchain version, as
per https://github.com/rust-lang/rustup#the-toolchain-file, we can make
this more specific (following
https://github.com/rust-lang/rustup#toolchain-specification) if that
turns out useful.
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