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

Make cargo export CARGO_TARGET_DIR to build.rs scripts #7325

Closed
wants to merge 3 commits into from

Conversation

cbeck88
Copy link

@cbeck88 cbeck88 commented Sep 4, 2019

Resolves issue #5457

#5457

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2019
@Eh2406
Copy link
Contributor

Eh2406 commented Sep 4, 2019

Thank you for getting this conversation started. Looks like CI is failing. This will need to be added to the docs.

The bigger question is how does this work with the direction we are hoping to take the structure of the target directory. @ehuss Thoughts?

@cbeck88
Copy link
Author

cbeck88 commented Sep 4, 2019

yeah i got a build failure, sorry, will iterate

@cbeck88
Copy link
Author

cbeck88 commented Sep 4, 2019

so i can try to describe my use-case but it's not unlike what people describe here: #5457

for my purpose, an alternate solution might be, instead make it so that build.rs can do:

println!("cargo:artifact={}/foo.so", out_dir);

and that would tell cargo that something that we produced and placed in out_dir should get copied to the target dir, whatever it may be

it's less immediately obvious to me how to write that patch though, but it also can't be that hard, i guess it's probably even in the same core/compiler/custom_build.rs file

afaik the only reason to do that instead of this is if there is some abstraction layer you don't want to break for some reason about build scripts and CARGO_TARGET_DIR

but I can't see what that would be, and the target dir is removed by cargo clean anyways, so build scripts putting stuff directly there doesn't seem so bad.

anyways would love to hear what you think

@alexcrichton
Copy link
Member

Thanks for the PR here @cbeck88, but the first thing that needs to be covered is whether the Cargo team thinks this should happen. Build scripts are intended to have a very scoped output (just in their OUT_DIR), and they're not currently really intended to be used as installation scripts for other sorts of files. In that sense this change was never intended for build scripts.

Now that's not to say that this can't happen, but this needs to be considered carefully. For example how would dependencies make use of this? Should this only be set for the "final package"? Is this really all that's necessary for installation? (etc, etc)

@cbeck88
Copy link
Author

cbeck88 commented Sep 5, 2019

thanks @alexcrichton

@alexcrichton
Copy link
Member

I'm going to go ahead and close this because I don't think this feature is fleshed out enough yet. I think that this still wants discussion to figure out the best way forward and how Cargo can cater to the use cases mentioned here and in #5457, and I don't think setting this one environment variable is going to be the full fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants