-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Reorganize build unit directory layout for new build-dir layout #16542
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?
Conversation
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/deps/foo[..][EXE] | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/deps/foo[..].d | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo.txt | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/build_script_build[..].d | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/build_script_build[..][EXE] | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/build-script-build[EXE] | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..][EXE] | ||
| [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..].d |
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.
note on the diffs here: Now that both the build-script and build units use the same directory we run into the issue of
foo/[HASH]/out/foo[..][EXE]
matching foo.txt that is output from the build-script.
To solve this I moved the foo.txt before the foo[..][EXE] line so it would be matched earlier.
But after doing that I felt the order of the files was mislead as it makes it look like foo.txt and foo.txt are in the same directory which they are not. (HASH is different)
So I also moved the other build script files in hopes that it might be a bit more clear.
cargo-semver-checks already supported rustdoc JSON v57 but not yet released
But the |
src/cargo/core/compiler/layout.rs
Outdated
| /// New features should consider using this so we can avoid their migrations. | ||
| pub fn deps_new_layout(&self, pkg_dir: &str) -> PathBuf { | ||
| self.build_unit(pkg_dir).join("deps") | ||
| pub fn output(&self, pkg_dir: &str) -> PathBuf { |
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.
Why is this output and not out?
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.
Sorry, I had original named them output but then changed the named to out and forgot to update this one.
Will change to out
| pub fn out_dir(&self, unit: &Unit) -> PathBuf { | ||
| let dir = self.pkg_dir(unit); | ||
| self.layout(unit.kind).build_dir().deps(&dir) |
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.
I feel like this is going to get confusing with us sometimes calling deps out_dir or deps
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.
err yeah, self.layout(...).build_dir().deps(...) should really be self.layout(...).build_dir().out(...).
This issue it that we already have an BuildDirLayout::out() that only supports the new layout.
I'll rename the current out() to out_force_new_layout() and deps() to out() that way everything is consistent
| pub fn build_script_out_dir(&self, unit: &Unit, is_new_layout: bool) -> PathBuf { | ||
| if is_new_layout { | ||
| self.out_dir(unit) | ||
| } else { | ||
| self.build_script_run_dir(unit).join("out") | ||
| } | ||
| } |
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.
When implementing unstable features, I'd recommend considering the process for removal on stabilization. Here, we will delete this function and update all callers to call out_dir, removing the parameter as w do so. Instead, if we put the conditional in the caller, then it is just a matter of deleting 4 lines in each call site, with it being obvious what to do.
I feel like I'd just put this conditional in the caller so it becomes straighforward to identify that we need to delete one branch and exclusively call one 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.
sure, that works for me. I'll pull the feature flag out of this fn to the callsite.
d7c6ce1 to
494c2f1
Compare
This comment has been minimized.
This comment has been minimized.
494c2f1 to
2decb74
Compare
This comment has been minimized.
This comment has been minimized.
2decb74 to
49abc7e
Compare
This comment has been minimized.
This comment has been minimized.
|
See cd39acf in commit history. Rebase got messed up? |
|
ahhh yeah, I think I did an |
Rename `fn out_dir` to `fn output_dir` to avoid naming collisions in future layout changes.
Previously `deps` was only the compiler output. This commit renames it to `out` to make it more general to any kind of build output. The build script OUT_DIR will eventually be merged into this directory.
This was previously indirectly created by `OUT_DIR`, however in the future `OUT_DIR` will not be nested in the build-script run dir so we always want to create it.
This commit moves the build-script OUT_DIR location from `{build-unit-dir}/build-script-execution/out`
to `{build-unit-dir}/out` dir (that was previously `{build-unit-dir}/deps`)
This commit changes the build script execution dir from `{build-unit-dir}/build-script`
to `{build-unit-dir}/run`. The motivation behind this is to have a
general directory for units that execute some binary/external process
and output to stdout/err. Currently this is only used by build-scripts
but could be expanded in the future.
49abc7e to
5716af9
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
What does this PR try to resolve?
This PR makes more changes to the new
build-dirlayout as discussed in #16502 (comment).The goal here is to have more general (and thus reusable) directories in the build unit structure.
Layout changes
{build-unit-dir}/depsto{build-unit-dir}/outOUT_DIRfrom{build-unit-dir}/build-script/outto{build-unit-dir}/out{build-unit-dir}/build-scriptto{build-unit-dir}/runThe resulting structure looks like
Part of #15010
How to test and review this PR?
See the test updates included in each commit