Refactor Layout into BuildDirLayout and ArtifactDirLayout
#16092
+170
−99
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,38 +111,8 @@ use std::path::{Path, PathBuf}; | |
| /// | ||
| /// See module docs for more information. | ||
| pub struct Layout { | ||
| artifact_dir: ArtifactDirLayout, | ||
| build_dir: BuildDirLayout, | ||
| } | ||
|
|
||
| impl Layout { | ||
|
|
@@ -182,9 +152,10 @@ impl Layout { | |
| // For now we don't do any more finer-grained locking on the artifact | ||
| // directory, so just lock the entire thing for the duration of this | ||
| // compile. | ||
| let artifact_dir_lock = | ||
| dest.open_rw_exclusive_create(".cargo-lock", ws.gctx(), "build directory")?; | ||
|
|
||
| let build_dir_lock = if root != build_root { | ||
| Some(build_dest.open_rw_exclusive_create( | ||
| ".cargo-lock", | ||
| ws.gctx(), | ||
|
|
@@ -201,23 +172,112 @@ impl Layout { | |
| let artifact = deps.join("artifact"); | ||
|
|
||
| Ok(Layout { | ||
| artifact_dir: ArtifactDirLayout { | ||
| dest: dest.clone(), | ||
| examples: dest.join("examples"), | ||
| doc: root.join("doc"), | ||
| timings: root.join("cargo-timings"), | ||
| _lock: artifact_dir_lock, | ||
| }, | ||
| build_dir: BuildDirLayout { | ||
| root: build_root.clone(), | ||
| deps, | ||
| build: build_dest.join("build"), | ||
| artifact, | ||
| incremental: build_dest.join("incremental"), | ||
| fingerprint: build_dest.join(".fingerprint"), | ||
| examples: build_dest.join("examples"), | ||
| tmp: build_root.join("tmp"), | ||
| _lock: build_dir_lock, | ||
| is_new_layout, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| /// Makes sure all directories stored in the Layout exist on the filesystem. | ||
| pub fn prepare(&mut self) -> CargoResult<()> { | ||
| self.artifact_dir.prepare()?; | ||
| self.build_dir.prepare()?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn artifact_dir(&self) -> &ArtifactDirLayout { | ||
| &self.artifact_dir | ||
| } | ||
|
|
||
| pub fn build_dir(&self) -> &BuildDirLayout { | ||
| &self.build_dir | ||
| } | ||
| } | ||
|
|
||
| pub struct ArtifactDirLayout { | ||
| /// The final artifact destination: `<artifact-dir>/debug` (or `release`). | ||
| dest: PathBuf, | ||
| /// The directory for examples | ||
| examples: PathBuf, | ||
| /// The directory for rustdoc output | ||
| doc: PathBuf, | ||
| /// The directory for --timings output | ||
| timings: PathBuf, | ||
| /// The lockfile for a build (`.cargo-lock`). Will be unlocked when this | ||
| /// struct is `drop`ped. | ||
| _lock: FileLock, | ||
| } | ||
|
|
||
| impl ArtifactDirLayout { | ||
|
There was a problem hiding this comment. We generally prefer impl blocks to be next to their types. There was a problem hiding this comment. I can understand keeping it here in this commit to make it easier to review but would be good to move in a follow up commit There was a problem hiding this comment. sure, added that change in 6751e46 |
||
| /// Makes sure all directories stored in the Layout exist on the filesystem. | ||
| pub fn prepare(&mut self) -> CargoResult<()> { | ||
| paths::create_dir_all(&self.examples)?; | ||
|
|
||
| Ok(()) | ||
| } | ||
| /// Fetch the destination path for final artifacts (`/…/target/debug`). | ||
| pub fn dest(&self) -> &Path { | ||
| &self.dest | ||
| } | ||
| /// Fetch the examples path. | ||
| pub fn examples(&self) -> &Path { | ||
| &self.examples | ||
| } | ||
| /// Fetch the doc path. | ||
| pub fn doc(&self) -> &Path { | ||
| &self.doc | ||
| } | ||
| /// Fetch the cargo-timings path. | ||
| pub fn timings(&self) -> &Path { | ||
| &self.timings | ||
| } | ||
| } | ||
|
|
||
| pub struct BuildDirLayout { | ||
| /// The root directory: `/path/to/build-dir`. | ||
| /// If cross compiling: `/path/to/build-dir/$TRIPLE`. | ||
| root: PathBuf, | ||
| /// The directory with rustc artifacts | ||
| deps: PathBuf, | ||
| /// The primary directory for build files | ||
| build: PathBuf, | ||
| /// The directory for artifacts, i.e. binaries, cdylibs, staticlibs | ||
| artifact: PathBuf, | ||
| /// The directory for incremental files | ||
| incremental: PathBuf, | ||
| /// The directory for fingerprints | ||
| fingerprint: PathBuf, | ||
| /// The directory for pre-uplifted examples: `build-dir/debug/examples` | ||
| examples: PathBuf, | ||
| /// The directory for temporary data of integration tests and benches | ||
| tmp: PathBuf, | ||
| /// The lockfile for a build (`.cargo-lock`). Will be unlocked when this | ||
| /// struct is `drop`ped. | ||
| /// | ||
| /// Will be `None` when the build-dir and target-dir are the same path as we cannot | ||
| /// lock the same path twice. | ||
| _lock: Option<FileLock>, | ||
| is_new_layout: bool, | ||
| } | ||
|
|
||
| impl BuildDirLayout { | ||
| /// Makes sure all directories stored in the Layout exist on the filesystem. | ||
| pub fn prepare(&mut self) -> CargoResult<()> { | ||
| if !self.is_new_layout { | ||
|
|
@@ -226,16 +286,10 @@ impl Layout { | |
| } | ||
| paths::create_dir_all(&self.incremental)?; | ||
| paths::create_dir_all(&self.examples)?; | ||
| paths::create_dir_all(&self.build)?; | ||
|
|
||
| Ok(()) | ||
| } | ||
| /// Fetch the deps path. | ||
| pub fn deps(&self, pkg_dir: &str) -> PathBuf { | ||
| if self.is_new_layout { | ||
|
|
@@ -248,22 +302,13 @@ impl Layout { | |
| pub fn legacy_deps(&self) -> &Path { | ||
| &self.deps | ||
| } | ||
| pub fn root(&self) -> &Path { | ||
| &self.root | ||
| } | ||
| /// Fetch the build examples path. | ||
| pub fn examples(&self) -> &Path { | ||
| &self.examples | ||
| } | ||
| /// Fetch the incremental path. | ||
| pub fn incremental(&self) -> &Path { | ||
| &self.incremental | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The separation of
ArtifactDirLayoutandBuildDirLayoutmakes sense, as it the actually layout of two directories a lot clearer.However, I feel like it should be opaque to anything outside layout.rs. Other module shouldn't concern where those build caches are actually stored. Exposing
fn artifact_dir()andbuild_dir()seems to expose a bit too many implementation details to me. What do you think?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 understand where you are coming from but I think exposing artifact_dir vs build_dir is necessary.
We already do that by duplicating functions like
examples()andbuild_examples(). If we keep everything inLayoutwe would probably eventually end up withroot()andbuild_root(),dest()andbuild_dest(), etc.At that point, I think its better to explicitly use the
build_dir()/artifact_dir()to avoid confusion as to where the files are going.Also I found that in most (all?) of the time I knew which directory I wanted move files to and the opaque nature of
Layoutfelt like it was hurting more than it was helping.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.
For myself, I feel like exposing the split isn't about exposing implementation details but being clearer in communication and thinking about intermediate and final artifacts.
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.
Fair! Thanks for the discussion!