Use environment variable for force rustup toolchain version #322

alevy · 2017-03-22T18:29:59Z

rustup looks at the RUSTUP_TOOLCHAIN variable first to figure out
which toolchain to use (e.g. nightly-2017-01-25). Using this
environment variable avoids requiring developers building the kernel to
configure toolchain themselves.

Solves #321

ppannuto · 2017-03-22T18:36:53Z

Makefile.common

@@ -33,7 +35,7 @@ endif

 # Check that rustc is the right nightly - warn not error if wrong, sometimes useful to test others
 RUSTC_VERSION_STRING := rustc 1.16.0-nightly (83c2d9523 2017-01-24)
-ifneq ($(shell rustc --version),$(RUSTC_VERSION_STRING))
+ifneq ($(shell RUSTUP_TOOLCHAIN=$(RUSTUP_TOOLCHAIN) rustc --version),$(RUSTC_VERSION_STRING))
    $(warning Unexpected rustc version. Expected $(RUSTC_VERSION_STRING) got $(shell rustc --version))
    $(warning You may experience unexpected compilation warnings or errors)
    $(warning To fix, run `rustup override set nightly-2017-01-25`)


Should we change this line to just suggest installing rustup instead of suggesting running override?

Also, if I've done an override and this env variable is set - who wins? Importantly, if the env variable wins, we need to have an escape hatch to let people try building with different versions briefly.

Added an escape hatch (will squash the commit):

$ make RUSTUP_VERSION=some-other-version

Suggest some best practice in place of the To fix language?

something like - "To fix Install rustup. Remove any overrides you may have set if rustup was previously installed"

Done, but I just pointed to https://rustup.rs for installation. RUSTUP_TOOLCHAIN takes precedence over rustup override.

ppannuto · 2017-03-22T18:38:04Z

Leaving this note here for the annals of history. In make $(shell's do not inherit exported environment variables, but recipe commands do:

http://stackoverflow.com/questions/2838715/makefile-variable-initialization-and-export

bradjc

Seems to work great for me.

bradjc · 2017-03-22T19:25:29Z

You should update getting_started

ppannuto · 2017-03-22T19:31:21Z

Really all of these:

$ git grep override
doc/Getting_Started.md:Then override the default version of Rust to use for Tock by running the
doc/Getting_Started.md:$ rustup override set nightly-2017-01-25
vagrant/Vagrantfile:    source ~/.profile && cd /tock && rustup override set nightly-2017-01-25

`rustup` looks at the `RUSTUP_TOOLCHAIN` variable first to figure out which toolchain to use (e.g. `nightly-2017-01-25`). Using this environment variable avoids requiring developers building the kernel to configure toolchain themselves.

alevy · 2017-03-22T19:35:17Z

Done, but it turns out not to be as sweet as we thought, you still need to manually install that version (which override does automatically). It's still nice that you don't have to get the override in the exact right directory and the failure mode is a bit less baffling, so probably still useful.

bradjc · 2017-03-22T19:47:41Z

Can't make run rustup for you?

alevy · 2017-03-22T20:07:44Z

OK, how's that?

ppannuto · 2017-03-22T20:22:46Z

Makefile.common

-ifneq ($(shell rustc --version),$(RUSTC_VERSION_STRING))
-    $(warning Unexpected rustc version. Expected $(RUSTC_VERSION_STRING) got $(shell rustc --version))
+ifeq ($(shell rustup -V &> /dev/null && echo success),success)
+  DUMMY := $(shell rustup install $(RUSTUP_TOOLCHAIN))


I think this needs to fail / bail out here somehow if the install fails. Make doesn't care if the sub $(shell exits with an error code:

$ cat Makefile FOO := /bin/false all: echo hi [-bash] Wed 22 Mar 16:21 [/tmp/m] $ make echo hi hi

any thoughts on how to do this?

How about this:

RUSTC_VERSION_STRING := rustc 1.16.0-nightly (83c2d9523 2017-01-24) ifneq ($(shell rustup -V &> /dev/null && rustup install $(RUSTUP_TOOLCHAIN) 1>&2 && echo success),success) ifneq ($(shell RUST_TOOLCHAIN=$(RUSTUP_TOOLCHAIN) rustc --version), $(RUSTC_VERSION_STRING)) $(warning You do not have the correct version of `rustc` configured, and `rustup` is not installed.) $(warning Expected `rustc` version "$(RUSTC_VERSION_STRING)" but got "$(shell rustc --version)") $(warning You may experience unexpected compilation warnings or errors) $(warning To fix, install `rustup` from https://rustup.rs/, then run: `rustup install $(RUSTUP_TOOLCHAIN)`) endif endif

Like the new language

It's a bit hacky, but the solution that jumps to mind is to just nest the checks:

ifeq ($(shell rustup -V &> /dev/null && echo success),success) DUMMY := $(shell rustup install $(RUSTUP_TOOLCHAIN)) ifneq ($(shell RUST_TOOLCHAIN=$(RUSTUP_TOOLCHAIN) rustc --version), $(RUSTC_VERSION_STRING)) $(error rustup failed. SAD)

Oic... how about this:

the logic is:

check to see if the expected version of rustc is installed (we don't care how, on the path? cool. from rustup? cool).

if not, just blindly try installing with rustup.

check again

if after all that, it's not installed, spit out the warning.

It involves the same check twice, but meh...

RUSTC_VERSION_STRING := rustc 1.16.0-nightly (83c2d9523 2017-01-24) ifneq ($(shell RUST_TOOLCHAIN=$(RUSTUP_TOOLCHAIN) rustc --version), $(RUSTC_VERSION_STRING)) DUMMY := $(shell rustup install $(RUSTUP_TOOLCHAIN)) ifneq ($(shell RUST_TOOLCHAIN=$(RUSTUP_TOOLCHAIN) rustc --version), $(RUSTC_VERSION_STRING)) $(warning You do not have the correct version of `rustc` configured, and `rustup` is not installed.) $(warning Expected `rustc` version "$(RUSTC_VERSION_STRING)" but got "$(shell rustc --version)") $(warning You may experience unexpected compilation warnings or errors) $(warning To fix, install `rustup` from https://rustup.rs/, then run: `rustup install $(RUSTUP_TOOLCHAIN)`) endif endif

Yeah, that looks perfect

bradjc · 2017-03-22T20:29:44Z

What if we make our getting_started specify that rustup is the dependency, and not rust. The we can update just with the makefile.

alevy · 2017-03-23T19:55:16Z

OK, took another pass. Whoever merges, if it's ready, please squash the commits?

ppannuto · 2017-03-23T19:58:57Z

Nice @alevy, thanks -- sorry to turn a two-line change into so much work 🤕

alevy requested a review from ppannuto March 22, 2017 18:30

ppannuto requested changes Mar 22, 2017

View reviewed changes

bradjc approved these changes Mar 22, 2017

View reviewed changes

alevy force-pushed the rustup branch from e84dcdd to 917eed3 Compare March 22, 2017 19:22

alevy force-pushed the rustup branch from 917eed3 to 5260044 Compare March 22, 2017 19:31

alevy added 2 commits March 22, 2017 15:32

Update Getting Started to match new rustup install method

3ef4932

Update Vagrant file for new rustup setup

e245b0c

Automatically install correct version with rustup

d7829f1

ppannuto reviewed Mar 22, 2017

View reviewed changes

Streamline auto-rustup step in Makefile

2096b27

bradjc merged commit 050d9f1 into tock:master Mar 23, 2017

alevy deleted the rustup branch March 23, 2017 19:59

ppannuto mentioned this pull request Mar 24, 2017

Add a make check target #328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use environment variable for force rustup toolchain version #322

Use environment variable for force rustup toolchain version #322

alevy commented Mar 22, 2017

ppannuto Mar 22, 2017

alevy Mar 22, 2017

ppannuto Mar 22, 2017

alevy Mar 22, 2017

ppannuto commented Mar 22, 2017

bradjc left a comment

bradjc commented Mar 22, 2017

ppannuto commented Mar 22, 2017

alevy commented Mar 22, 2017

bradjc commented Mar 22, 2017

alevy commented Mar 22, 2017

ppannuto Mar 22, 2017

alevy Mar 23, 2017

alevy Mar 23, 2017

ppannuto Mar 23, 2017

alevy Mar 23, 2017

ppannuto Mar 23, 2017

bradjc commented Mar 22, 2017

alevy commented Mar 23, 2017

ppannuto commented Mar 23, 2017

Use environment variable for force rustup toolchain version #322

Use environment variable for force rustup toolchain version #322

Conversation

alevy commented Mar 22, 2017

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

ppannuto commented Mar 22, 2017

bradjc left a comment

Choose a reason for hiding this comment

bradjc commented Mar 22, 2017

ppannuto commented Mar 22, 2017

alevy commented Mar 22, 2017

bradjc commented Mar 22, 2017

alevy commented Mar 22, 2017

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

bradjc commented Mar 22, 2017

alevy commented Mar 23, 2017

ppannuto commented Mar 23, 2017