-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[WIP] Accessing each build script's OUT_DIR
#15891
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,7 +210,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { | |
plan.output_plan(self.bcx.gctx); | ||
} | ||
|
||
// Add `OUT_DIR` to env vars if unit has a build script. | ||
// Add `OUT_DIR` to runtime env vars if unit has a build script. | ||
let units_with_build_script = &self | ||
.bcx | ||
.roots | ||
|
@@ -227,11 +227,22 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { | |
.display() | ||
.to_string(); | ||
let script_meta = self.get_run_build_script_metadata(&dep.unit); | ||
self.compilation | ||
|
||
// It creates environment variable named build-script-<name>_OUT_DIR | ||
let mut out_dir_name = dep.unit.target.name().to_owned(); | ||
out_dir_name = out_dir_name | ||
.strip_prefix("build-script-") | ||
.unwrap() | ||
.to_string(); | ||
out_dir_name.push_str("_OUT_DIR"); | ||
|
||
let env = self | ||
.compilation | ||
.extra_env | ||
.entry(script_meta) | ||
.or_insert_with(Vec::new) | ||
.push(("OUT_DIR".to_string(), out_dir)); | ||
.or_insert_with(Vec::new); | ||
env.push(("OUT_DIR".to_string(), out_dir.clone())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll get back to this |
||
env.push((out_dir_name, out_dir)); | ||
Comment on lines
+239
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a separate commit maybe? Or should I update the latest commit itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless the commit is ginormous (e.g. adding all of |
||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1706,10 +1706,15 @@ fn build_deps_args( | |
|
||
for dep in deps { | ||
if dep.unit.mode.is_run_custom_build() { | ||
cmd.env( | ||
"OUT_DIR", | ||
&build_runner.files().build_script_out_dir(&dep.unit), | ||
); | ||
let out_dir = &build_runner.files().build_script_out_dir(&dep.unit); | ||
let mut out_dir_name = dep.unit.target.name().to_owned(); | ||
out_dir_name = out_dir_name | ||
.strip_prefix("build-script-") | ||
.unwrap() | ||
.to_string(); | ||
out_dir_name.push_str("_OUT_DIR"); | ||
cmd.env("OUT_DIR", &out_dir); | ||
cmd.env(&out_dir_name, &out_dir); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't forget to update the test from 6c8bc08 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I added an extra test for duplicated/conflicting declarations of same function |
||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saying
runtime
here might be confusing as we are adding it for use at compile time of the package and not forcargo run
orcargo test
running binaries.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, this is us setting the specialized
OUT_DIR
on the build script itself? We probably shouldn't be doing that. Depending on how we do build script delegation, the name will be dependent on how the caller sees the build script which will make it undiscoverable for build scripts to use.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. While this could make it easier to match up build script to rust source, it runs into the problem mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the earlier code, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you are asking about. We should only set the new environment variable on the libs/bins/etc and not on the build script execution.