Possibility of setting cargo key:value on commandline

[emoon]

Hi,

I'm adding support for Cargo it the Tundra build system here https://github.com/emoon/tundra/tree/rust-support

The reason is that I have a bunch of native code in Tundra already but I want to build some Rust code as well and also link with static libs produced by Tundra. Another reason is that I will have several Cargo executables/shared libs and Tundra can schedule them in parallel which would speed up my builds a bit.

Now the problem is that (from what I understand) there is no way to set Cargo key:value (the same way build.rs would output to stdio) on command-line. Which causes some issues:

My current approach will be to generate/patch an existing build.rs file for the Cargo project and adding all the libs that needs to linked to the executable/shared lib. I feel that this is somewhat hacky and has one issue that Tundra supports building debug/release configuration at the same time so there will be a race condition here if more than one thread is trying to update the build.rs file. With command line parameters this wouldn't be an issue.

Also setting the cargo target dir is possible using the CARGO_TARGET_DIR env which I currently do so from so that the build command being executed from tundra looks like this

"set CARGO_TARGET_DIR=some_dir ; cargo build ..."

Now this works fine but having a command-line option like --target-dir=.../ to Cargo directly would be a bit nicer.

Another issue which I have seen (when using a build.rs) is that if dependencies update (like staticlibs) Cargo doesn't seem to know about it and will not link again. So a --force-link flag would be nice as well.

So to summarize. Something like this would be awesome :)

--set=key:value
--set-target-dir=..
--force-link

[alexcrichton]

I think that we may want to take this one at a time, or perhaps split up this issue a bit (wouldn't want too much unrelated discussion here!).

First up, about key:value pairs. These are actually overridable via configuration. I do agree that sometimes having command line arguments is indeed nicer, but this can often be problematic for Cargo. For example a key/value pair can't just be injected into the commandline, but rather it needs to be injected into a specific package's build script. Not only that but it needs to indicate what target triple it's for as well. Currently, while somewhat unergonomic, the configuration option handles all of these cases.

It's also somewhat intentional for now that the target directory is an environment variable rather than a CLI option. It's likely to be something that's persisted among many invocations of Cargo rather than present for just once, hence an environment variable perhaps being a little more appropriate. That being said though I'd be fine adding this option.

Finally, for forcing Cargo to actually perform the link step. Cargo in general tries to track dependencies of builds wherever possible, and the recent addition of the rerun-if-changed key has added even more of an ability to do this. Cargo unfortunately will unlikely know all inputs to a build (such as system libraries changing), but it certainly tries its best! To force a build you can either do a cargo clean, cargo clean -p, or remove the artifact manually, however.

[emoon]

Hi,

Thanks for the reply. I will try not to split right now (I think it work out, lets try at least :)

First up, about key:value pairs. These are actually overridable via configuration. I do agree that sometimes having command line arguments is indeed nicer, but this can often be problematic for Cargo. For example a key/value pair can't just be injected into the commandline, but rather it needs to be injected into a specific package's build script. Not only that but it needs to indicate what target triple it's for as well. Currently, while somewhat unergonomic, the configuration option handles all of these cases.

Alright. Yeah in my case as I will add dependencies from "the outside" I will likely just need to let the build system update the build.rs file then. This isn't a major issue but just feels a bit klunky but I can live with that (alternative would be for the user to manually add the dependencies but I would rather go for the first alternative then)

It's also somewhat intentional for now that the target directory is an environment variable rather than a CLI option. It's likely to be something that's persisted among many invocations of Cargo rather than present for just once, hence an environment variable perhaps being a little more appropriate. That being said though I'd be fine adding this option.

Yeah this isn't a major issue really as I can set this from the outside. In my case I will need to have separate directories as I have have several Cargo instances running at the same time and from what I understand is that it doesn't work good to have them all poking around in the same directory at the same time.

Finally, for forcing Cargo to actually perform the link step. Cargo in general tries to track dependencies of builds wherever possible, and the recent addition of the rerun-if-changed key has added even more of an ability to do this. Cargo unfortunately will unlikely know all inputs to a build (such as system libraries changing), but it certainly tries its best! To force a build you can either do a cargo clean, cargo clean -p, or remove the artifact manually, however.

Didn't know about the rerun-if-changed key so that might work. I had a case where an external lib (which was specified in build.rs) was updated but when running Cargo it wouldn't re-link even if there had been a change in a dependency so that is why I figured a "re-link" option would have been good as I know that a dependency has been updated so a re-link (or a regular build) needed to be done.

But it seems I can work-around most of these issues anyway so I should really be fine.

[emoon]

Ok now that I think about it we can ignore the env and force-link issue right now (I will do as you suggested an open new issues for those cases if needed)

What would be good is that if you could specific a build.rs file on command line instead of doing the key:value thing. That would work solve my biggest issues.

The way Tundra works is that it builds a DAG on all build steps and it's dependencies and it's expected that it build step is atomic for a given configuration (debug, release, etc) so I don't want to poke around in build.rs while several cargo instances may be using it. The reason is that Tundra can build several configurations in parallel at the same time which may be faster depending on how the build looks like.

A work-around for this would be during the DAG setup do a copy of Cargo.toml, and change the build = "build.rs" to something like "build_debug.rs" , add all debug dependencies to that file, and then do the same thing for the rest of the configurations.

So debug step would be:

cargo build --manifest-file=some/dir/Cargo_debug.toml

Release

cargo build --manifest-file=some/dir/Cargo_release.toml

With a new command-line parameter it would look something like

cargo build --manifest-file=some/dir/Cargo.toml --build=temp/debug.rs
cargo build --manifest-file=some/dir/Cargo.toml --build=temp/release.rs

Now build systems from the outside could setup dependencies that cargo needs to link with quite easily without touching the Cargo.toml file.

[alexcrichton]

add dependencies from "the outside"

Could you elaborate a bit on what the motivation here is? I'm curious to understand this a bit more so Cargo may hopefully help out in the future more as well.

as I have have several Cargo instances running at the same time

Yeah this is covered by #354, but you'll also want to ensure that concurrent Cargo instances have disjoint home directories as well.

---

Ah it looks like while I was typing this up a new response was made as well! I'm a little confused by the debug/release build script support, are you expecting this to work for arbitrary packages? That'd change the landscape of what's going on here slightly I believe.

I guess I'm just not quite understanding what's up with these key/value pairs and why the build system wants to change them so radically? What's the purpose here in terms of the end goal?

[emoon]

Of course!

require "tundra.syntax.rust-cargo"

StaticLibrary {
    Name = "native_lib",
    Sources = { "native_lib/lib.c" },
}

RustProgram {
    Name = "test",
    CargoConfig = "test/Cargo.toml",
    Sources = { "test/src/main.rs" },
    Depends = { "native_lib" },
}

Default "test"

This is how you configure a build in Tundra (excluding some details)

Now imagine that "native_lib" here is way more complicated than in this example (otherwise you could just use regular build.rs with gcc create)

RustProgram here is calling cargo behind the scenes but in order for Cargo to know about linking with the native lib it needs to tell Cargo that is some way. Today (afaik) the only way to do that is to do something with the build.rs file to add the dependencies. That is what I mean dependencies from "the outside" Hopefully that makes it a bit more clear.

[emoon]

Yeah this is covered by #354, but you'll also want to ensure that concurrent Cargo instances have disjoint home directories as well.

In my case git config is in the root of the project (not inside each Cargo project) so I should be fine with that (and target directories should be different) I guess Cargo.lock would be a problem if several instances fight over it but it general I think I should be good (I hope :)

[alexcrichton]

Ah yes thanks for the explanation, that indeed helps! Right now Cargo is geared towards helping each crate being an independent unit that's reusable elsewhere, so it isn't currently tailored towards use cases like this. In the example you gave it looks like the Rust library may not be able to build on its own (e.g. it must be built with this build system), right?

If that's the case, you can add something like this to the manifest:

build = "build.rs"
links = "native_lib"

and then you can use the standard system of overrides to provide the information from the outside about native_lib

[emoon]

Hi,

Thanks for the reply.

Yes in this case the Rust program must be built by the build system (as the Rust program depends on native library to work) One of the reasons I want to do this is to run a bunch of post steps with the build system also (it can be package a build, running various tests, etc) Also I well have several crates (shared libs which acts as plugins as for the main program) being compiled at the same time as it reduces the total compilation time.

About build.rs/links:

This would work. The only issue is that you have to make sure to not forget it (as you add dependency it two places, one in the Cargo.toml and one in the Tundra config) but that might not be such a huge deal.

and then you can use the standard system of overrides to provide the information from the outside about native_lib

By standard do you mean

cargo:libdir=/path/to/foo/lib

Which one would set inside build.rs to tell Cargo where to look for the library in question or something else?

[emoon]

I actually figured out a way around this.

What I will do is simply to set a bunch of extra env variables before I run Cargo (for example the output directory of where all native libs are and another env with all the libs that needs to be linked to.) Then in build.rs I will have something like this

fn main() {
    let tundra_dir = env::var("TUNDRA_OBJECTDIR").unwrap();
    let libs = env::var("TUNDRA_LIBS").unwrap();
    let native_libs = libs.split(" ");

    println!("cargo:rustc-link-search=native={}", tundra_dir);

    for lib in native_libs {
        println!("cargo:rustc-link-lib=static={}", lib);
    }
}

[alexcrichton]

@emoon yeah that's possible (as a build script), or I was thinking you could also do something like generate a .cargo/config that looks like:

[target.x86_64-unknown-linux-gnu.native_lib]
rustc-link-search = ['...']
rustc-link-lib = ['...']

[emoon]

Ah yeah that would also work. I will close this issue now as I have ways to solve this.

Thanks a lot for the help!

[alexcrichton]

No problem, and feel free to let me know if there's anything else I can do to help!

[emoon]

Will do.

Thanks again for the great work on Cargo. It's really an awesome tool :)

[emoon]

Hi again,

I have implemented all of this and it works great on Mac without any issues but it doesn't work on Windows because when I do this

set CARGO_TARGET_DIR=some_dir && cargo build --manifest-path=test/Cargo.toml

I get

couldn't prepare build directories

Caused by:
  The system cannot find the path specified. (os error 3)

If I run the set command first and then Cargo it works... So I figured this may be some Windows issue but I wrote a small C program that did the same thing so

set CARGO_TARGET_DIR=some_dir && my_prog

my_prog can find CARGO_TARGET_DIR without any issues so I wonder if you have any idea what's going on? It's very easy to repro

cargo new test
set CARGO_TARGET_DIR=foo && cargo build --verbose --manifest-path=test/Cargo.toml

If I do this on Mac it works just fine

cargo new test
export CARGO_TARGET_DIR=foo ; cargo build --verbose --manifest-path=test/Cargo.toml

[emoon]

@alexcrichton I tried to get Cargo to compile on Windows so I could take a look at the issue but gave up. Now when Rustc is about to use Cargo to build maybe it would be possible in the future to use Cargo to build Cargo? :)

[alexcrichton]

Oh Cargo's actually been bootstrapped with Cargo itself almost since day 1, a vanilla cargo build should do basically everything in a checkout of this repo.

I wonder if perhaps CARGO_TARGET_DIR needs to be an absolute path on Windows? otherwise I'm not really sure what's going on here unfortunately, do you have a repro that I could try (including a project)? Also, you're running this from cmd.exe?

[emoon]

I found the issue. It seems if one do

set CARGO_TARGET_DIR=something && ...

The result actually is

CARGO_TARGET_DIR="something " <- notice trailing space

DOH!

So doing

set CARGO_TARGET_DIR=something&&cargo build...

Works as it should but now I run into another issue where Cargo say:

Could not execute process "rustc ...."
Caused by:
  no stdio handle available for this process

[emoon]

Oh Cargo's actually been bootstrapped with Cargo itself almost since day 1, a vanilla cargo build should do basically everything in a checkout of this repo.

Oh! Nice :) I was reading this section https://github.com/rust-lang/cargo#compiling-from-source and it didn't mentioned that so I figured that it didn't work.

[alexcrichton]

Aha interesting! I wonder if the Windows shell is doing something funny? Or maybe rustc is something like a wrapper script? That seems like an unusual error...

Also that's a good point about the documentation, I'll add some notes there that cargo build should work just fine nowadays.

[emoon]

Yeah so I suspect that Tundra is doing something that makes Cargo unable to get a stdio handle but I have no idea why. I will poke the Tundra author about it.

And yes, building Cargo with Cargo worked great :) (One needed to install the Windows 10 SDK but there was a good error about it)

[emoon]

Ok I have been diving around now a bit regarding this.

Tundra sets Stdin to NULL (message here deplinenoise/tundra#261 (comment))

And when Cargo uses the standard setup for spawning a process then this code

https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/process.rs#L147

        let stdin = try!(in_handle.to_handle(c::STD_INPUT_HANDLE));

This will fail as it assumes Stdin should be present.

So I decided to poke around a bit in the Cargo code and I tried this in process_builder.rs

    pub fn build_command(&self) -> Command {
        let mut command = Command::new(&self.program);
        command.stdin(Stdio::null()); // <--- NEW CODE
        if let Some(cwd) = self.get_cwd() {
            command.current_dir(cwd);
        }

Now everything seems to work as expected. I have no idea if this change is sound or not. Running all the tests the result is identical to without the change (I have 516 passed and 15 failed on my system)

I would love to hear your input on this as I'm not really sure how to proceed.

[alexcrichton]

Aha that definitely sounds like a bug in "handle inheritance" in Rust process spawning. I've opened up a bug on the rust-lang/rust repo

[emoon]

Thanks!