Ensure that the specified rustup toolchain exists before using it #388

Ma27 · 2020-03-19T20:50:20Z

Given a config like this

[tasks.test]
command = "cargo"
args = [ "build" ]
toolchain = "alarm"

you get a fairly unintuitive error output like this:

[cargo-make] INFO - Running Task: test
[cargo-make] ERROR - Error while executing command, exit code: 1
[cargo-make] WARN - Build Failed.

This can be quite confusing especially for people who are new to a
project and might've missed installing the specified rustup toolchain
before building the project with cargo-make.

This patch ensures in the toolchain-subcrate that the toolchain
actually exists (by checking the exit-code of rustup run <toolchain> true) before running the actual task. If the toolchain doesn't exist,
an error like this will be thrown:

[cargo-make] INFO - Running Task: test
[cargo-make] ERROR - Please install the toolchain alarm using rustup before building!
[cargo-make] WARN - Build Failed.

sagiegurari · 2020-03-19T21:14:38Z

that's awesome. thanks a lot.
I'll review the PR but overall i think its really good idea.

sagiegurari

looks really great. i think we need to fix the tests as they are failing and we can fix them easily and make sure they run for all rust versions.

src/lib/toolchain.rs

src/lib/toolchain_test.rs

codecov-io · 2020-03-20T18:23:55Z

Codecov Report

Merging #388 into 0.30.0 will decrease coverage by 0.26%.
The diff coverage is 30.95%.

@@            Coverage Diff             @@
##           0.30.0     #388      +/-   ##
==========================================
- Coverage   92.28%   92.02%   -0.27%     
==========================================
  Files          86       86              
  Lines       17025    16993      -32     
==========================================
- Hits        15711    15637      -74     
- Misses       1314     1356      +42

Impacted Files	Coverage Δ
src/lib/toolchain.rs	`100% <100%> (ø)`	⬆️
src/lib/toolchain_test.rs	`27.5% <14.7%> (-69.28%)`	⬇️
...c/lib/installer/rustup_component_installer_test.rs	`85.91% <0%> (-8.46%)`	⬇️
src/lib/command_test.rs	`85% <0%> (-8.34%)`	⬇️
src/lib/installer/cargo_plugin_installer.rs	`75.29% <0%> (-8.24%)`	⬇️
src/lib/installer/cargo_plugin_installer_test.rs	`90.44% <0%> (-3.38%)`	⬇️
src/lib/io.rs	`64.7% <0%> (-2.95%)`	⬇️
src/lib/condition.rs	`93.77% <0%> (-2.88%)`	⬇️
src/lib/legacy.rs	`70% <0%> (-2%)`	⬇️
src/lib/cache.rs	`61.29% <0%> (-1.62%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28718f4...f921772. Read the comment docs.

sagiegurari

looks great. i have no more comments on my end, just lets fix those 3 lines so tests will pass on all channels and i'll merge this.
you have hardcoded nightly in the results instead of comparring with channel variable as those tests run in stable/beta as well.

src/lib/toolchain_test.rs

Given a config like this [tasks.test] command = "cargo" args = [ "build" ] toolchain = "alarm" you get a fairly unintuitive error output like this: ``` [cargo-make] INFO - Running Task: test [cargo-make] ERROR - Error while executing command, exit code: 1 [cargo-make] WARN - Build Failed. ``` This can be quite confusing especially for people who are new to a project and might've missed installing the specified rustup toolchain before building the project with `cargo-make`. This patch ensures in the `toolchain`-subcrate that the toolchain actually exists (by checking the exit-code of `rustup run <toolchain> true`) before running the actual task. If the toolchain doesn't exist, an error like this will be thrown: ``` [cargo-make] INFO - Running Task: test [cargo-make] ERROR - Missing toolchain alarm! Please install it using rustup. [cargo-make] WARN - Build Failed. ```

sagiegurari · 2020-03-21T21:49:50Z

thanks a lot for the PR. i'm merging it now. don't worry about the build errors, i'll resolve them.

sagiegurari requested changes Mar 20, 2020

View reviewed changes

Ma27 force-pushed the rustup-toolchain branch from 52fe627 to e58f3a9 Compare March 20, 2020 18:23

sagiegurari changed the base branch from master to 0.30.0 March 21, 2020 08:58

sagiegurari requested changes Mar 21, 2020

View reviewed changes

src/lib/toolchain_test.rs Outdated Show resolved Hide resolved

src/lib/toolchain_test.rs Outdated Show resolved Hide resolved

src/lib/toolchain_test.rs Outdated Show resolved Hide resolved

sagiegurari assigned Ma27 Mar 21, 2020

sagiegurari added the enhancement label Mar 21, 2020

sagiegurari added this to the 0.30.0 milestone Mar 21, 2020

Ma27 force-pushed the rustup-toolchain branch from e58f3a9 to f921772 Compare March 21, 2020 18:40

sagiegurari merged commit f3abfed into sagiegurari:0.30.0 Mar 21, 2020

Ma27 deleted the rustup-toolchain branch March 21, 2020 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that the specified rustup toolchain exists before using it #388

Ensure that the specified rustup toolchain exists before using it #388

Ma27 commented Mar 19, 2020

sagiegurari commented Mar 19, 2020

sagiegurari left a comment

codecov-io commented Mar 20, 2020 •

edited

sagiegurari left a comment

sagiegurari commented Mar 21, 2020

Ensure that the specified rustup toolchain exists before using it #388

Ensure that the specified rustup toolchain exists before using it #388

Conversation

Ma27 commented Mar 19, 2020

sagiegurari commented Mar 19, 2020

sagiegurari left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 20, 2020 • edited

Codecov Report

sagiegurari left a comment

Choose a reason for hiding this comment

sagiegurari commented Mar 21, 2020

codecov-io commented Mar 20, 2020 •

edited