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

"toolchain" is not obeyed when running Duckscript #658

Closed
sffc opened this issue May 31, 2022 · 10 comments
Closed

"toolchain" is not obeyed when running Duckscript #658

sffc opened this issue May 31, 2022 · 10 comments
Assignees
Milestone

Comments

@sffc
Copy link

sffc commented May 31, 2022

Describe The Bug

The toolchain config option in Makefile.toml seems to only affect commands run directly with cargo. However, if I invoke the cargo tool from inside Duckscript, I would like the toolchain to also be applied.

To Reproduce

Add the following rules to your Makefile.toml:

[tasks.version-cargo]
command = "cargo"
toolchain = "nightly-2022-04-05"
args = ["version"]

[tasks.version-duckscript]
toolchain = "nightly-2022-04-05"
script_runner = "@duckscript"
script = '''
exec cargo version
'''

Actual behavior:

$ cargo make version-cargo
[cargo-make] INFO - cargo make 0.35.6
[cargo-make] INFO - Build File: Makefile.toml
[cargo-make] INFO - Task: version-cargo
[cargo-make] INFO - Profile: development
[cargo-make] INFO - Running Task: legacy-migration
[cargo-make] INFO - Execute Command: "rustup" "run" "nightly-2022-04-05" "cargo" "version"
cargo 1.62.0-nightly (1ef1e0a 2022-03-31)
[cargo-make] INFO - Build Done in 0.57 seconds.

$ cargo make version-duckscript
[cargo-make] INFO - cargo make 0.35.6
[cargo-make] INFO - Build File: Makefile.toml
[cargo-make] INFO - Task: version-duckscript
[cargo-make] INFO - Profile: development
[cargo-make] INFO - Running Task: legacy-migration
[cargo-make] INFO - Running Task: version-duckscript
cargo 1.61.0 (a028ae4 2022-04-29)
[cargo-make] INFO - Build Done in 0.55 seconds.

Expected behavior: both the first command and the second command should be using "cargo 1.62.0-nightly".

CC @Manishearth

@sagiegurari
Copy link
Owner

@sffc thanks for reporting.
I think the readme might not have been clear enough but this is only supported for commands, meaning 'command' attribute, not scripts (thats where i think i wasn't clear in my readme).
the issue you raise is valid, but if i fix duckscript, it would be confusing why its not working for simple shell scripts (where i can't possibly be able to fix).

as an alternative, i suggest to do something which is a bit messy (sorry).
my guess is that your script is not really a 1 liner and has a lot of logic and somewhere inside you call cargo somecommand.
So we need to extract it out of the script as follows:

[tasks.cargo-mycommand]
toolchain = "nightly-2022-04-05"
command = "cargo"
args = ["mycommand", "@@remove-empty(SOME_OPTIONAL_ARG)"]

[tasks.longscript]
script_runner = "@duckscript"
script = '''
#some really long script with lots of logic

# call cargo but with defined toolchain and maybe even pass some arg
set_env SOME_OPTIONAL_ARG hello
cm_run_task cargo-mycommand

#more script content that does amazing things
'''

@sffc
Copy link
Author

sffc commented Jun 1, 2022

Thanks for the reply!

my guess is that your script is not really a 1 liner and has a lot of logic and somewhere inside you call cargo somecommand.

That's basically correct, and your workaround should generally work, except in my case the invocation of cargo is actually hidden inside another Rust program. So cargo make calls duckscript which calls program which calls cargo somecommand. The inner program (which I did not write) checks the $CARGO environment variable to figure out which cargo to use.

I guess in general my expectation is that the toolchain argument should influence the toolchain used by any command in the script that follows, by resetting the environment variables as needed before invoking the script.

@sagiegurari
Copy link
Owner

@sffc got it.
so, no, it works differently.
it actually doesn't touch the env, but instead prepends the toolchain to the command as follows:

rustup run toolchainvalue originalcommand

so my workaround might actually work for that use case as well. no?

@sffc
Copy link
Author

sffc commented Jun 2, 2022

Hmm. I think part of the confusion is that rustup run does not override the environment variable $CARGO, so the cargo version running the script "leaks" into the environment of the script.

@sagiegurari
Copy link
Owner

lol. you are right.
let me think of something, i might have a solution.

@sagiegurari
Copy link
Owner

@sffc can you check development branch 0.53.13
It should define the CARGO env var for any script (not just duckscript) invoked from a task when a toolchain is defined.

[tasks.echo-cargo-stable]
toolchain = "stable"
script = '''
echo ${CARGO}
'''

[tasks.echo-cargo-nightly]
toolchain = "nightly"
script = '''
echo ${CARGO}
'''

[tasks.echo-cargo-all]
dependencies = ["echo-cargo-stable", "echo-cargo-nightly"]

@sagiegurari
Copy link
Owner

@sffc did you have a chance to verify the change?

@sagiegurari sagiegurari added this to the 0.35.13 milestone Jun 11, 2022
@sagiegurari
Copy link
Owner

this is now released. @sffc feel free to reopen if needed and thanks for notifying me of this issue

@sffc
Copy link
Author

sffc commented Jun 13, 2022

I updated cargo-make and can verify that invocations of cargo within duckscript are now using the correct cargo version. Thank you!

@sagiegurari
Copy link
Owner

thanks for checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants