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

Rustup integration for non-binary/non-cargo dependencies #139

Closed
roblabla opened this issue Nov 13, 2018 · 5 comments
Closed

Rustup integration for non-binary/non-cargo dependencies #139

roblabla opened this issue Nov 13, 2018 · 5 comments
Assignees
Milestone

Comments

@roblabla
Copy link
Contributor

Problem Description

The rustup integration right now seems to assumes two things:

  1. The rustup binary has an equivalent cargo crate. This is not always the case, for instance rustup has components like llvm-tools, which install llvm versions of objdump, strip, etc... Those are C dependencies, and as such are not available via cargo.

  2. The integration assumes that the dependency provides a binary. This is not always the case either, for instance the rust-src component creates a new directory containing the rust sources.

One use-case I have for this integration would be to install rust-src automatically on cargo make invocation, since it's a necessary dependency of cargo-xbuild. I don't think it's possible right now. Is there any plan for this?

@sagiegurari
Copy link
Owner

sorry for the loooong answer.
First of all, you are right. I didn't think of that use case when pushing the rustup integration.
so right now I have the following:

# install crate using cargo install using crate name
install_crate = "mycrate"

# install crate via rustup and fallback to cargo
install_crate = { crate_name = "mycrate", rustup_component_name = "rustup-component", binary = "testbinary", test_arg = "--help" }

# install crate via cargo install
install_crate = { crate_name = "mycrate", binary = "testbinary", test_arg = "--help" }

so you are looking for a way to install via rustup a source only component and components that you can't fallback to cargo install.
In your case, i think the crate is cargo-xbuild, no? so that would automatically be installed for you if you define the command as cargo and first arg as xbuild (if not already installed).
However, in your use case, you have additional dependencies to install and would want to have install_script invoked as well, something like

install_script = [ "rustup component add llvm-tools" ]

which is something you can do today and should work just fine. install_scripts are invoked if install_crate is not defined and do not have any 'automatic' conditions to them so that would work.
But they prevent the automatic crate installation based on your command (so cargo-xbuild would be missing).
In my mind, you only need to install 1 thing in a task, so its either install_crate, install_script or automatic from your command.

So those are 2 issues:

  1. enable to easily install rustup source components and components without crate fallback.
  2. you can only install one thing per task.

For item 1, we could modify install_crate to invoke only rustup without cargo fallback

# install crate via rustup without cargo install fallback
# binary and test arg would be optional and only used if provided to prevent unneeded installation
install_crate = { rustup_component_name = "llvm-tools", binary = "llvm-nm", test_arg = "--help" }
install_crate = { rustup_component_name = "rust-src" }

That should be really easy to develop for cargo-make.

As for item 2, you will need to do the following, since install_crate can be defined once in a task and since you have another crate there that might need to be installed (cargo-xbuild), you might want to split it to 2 tasks.
it will enable you to do multiple xbuild commands, all of which will depend on the 'installation' task so you can ensure it will be invoked only once per entire flow (as dependencies are invoked once)

[tasks.install-llvm-tools]
install_crate = { rustup_component_name = "llvm-tools", binary = "llvm-nm", test_arg = "--help" }

[tasks.install-rust-src]
install_crate = { rustup_component_name = "rust-src" }

[tasks.xbuild1]
# run cargo xbuild, if xbuild is not installed, it will be automatically installed for you
command = "cargo"
args = [ "xbuild", .... ]
dependencies = [ "install-llvm-tools", "install-rust-src" ]

[tasks.xbuild2]
# run cargo xbuild, if xbuild is not installed, it will be automatically installed for you
command = "cargo"
args = [ "xbuild", .... ]
dependencies = [ "install-llvm-tools", "install-rust-src" ]

[tasks.myflow]
dependencies = [ "xbuild1", "xbuild2" ]

that doesn't require any cargo-make changes.

summary:

  1. develop rustup installation without cargo fallback and optional binary check
  2. users will split multiple installation steps to multiple tasks.

makes sense?

@roblabla
Copy link
Contributor Author

Yes, this all makes sense! And yeah, for my use-case, I'd use this with cargo-xbuild or xargo. The plan you laid out should work well for me. I'll install the latest commit of cargo-make and give it a try :).

As a bit of a sub-issue: I think the documentation doesn't properly explain that the various auto-installation steps are mutually exclusive. I came to realize this by reading your above post. I think it'd be nice if this was explicitly mentioned at the beginning of the "installing dependencies" section. In addition to this, maybe the flow for having multiple "install" dependencies should be also explained in the documentation. It'll certainly come in handy for some people.

And as always, thanks for the quick reply and the awesome work on cargo-make!

@sagiegurari
Copy link
Owner

thanks :)

very curious to see if the latest changes work well for you, so please keep me updated.

as for docs, I totally agree. I wrote a lot, but still much is still missing. I'll definitely add the installation descriptions from above to the main readme.

@roblabla
Copy link
Contributor Author

roblabla commented Nov 14, 2018

Alright, just finished doing some tests.

rust-src and cargo-xbuild both get properly installed.

I tested llvm-tools-preview, and while it installs properly, it doesn't seem to be trying to run the test binary. E.G. install_crate = { rustup_component_name = "llvm-tools-preview", binary = "llvm-nm", test_arg = "--help" } doesn't check if the binary is properly installed by running llvm-nm.

As a separate issue, llvm-tools-preview can't be properly tested for presence anyways. That's because it isn't added to the $PATH (rustup run doesn't work either). Instead, we're supposed to use https://crates.io/crates/cargo-binutils to run them. So using llvm-tools-preview as an example in the README is a bit misleading. It might be better to show with the rls-preview compoent:

install_crate = { rustup_component_name = "rls-preview", binary = "rls", test_arg = "--help" }

I'm not sure the test for presence is really necessary anyways. If we're going to run a binary anyways, it doesn't matter if it's rustup component add or <binary> --help :P

@sagiegurari
Copy link
Owner

Great thanks. Good timing, as i'm writing the docs now :)

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