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.

Sign up for GitHub

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

Added support for target-dir in config file. #177

Closed
wants to merge 1 commit into from
Closed

Added support for target-dir in config file. #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/cargo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,15 @@ pub fn run(args: &Args, verbose: bool) -> Result<ExitStatus> {
}

pub struct Config {
root: PathBuf,
table: Value,
}

impl Config {
pub fn root(&self) -> &PathBuf {
&self.root
}

pub fn target(&self) -> Result<Option<&str>> {
if let Some(v) = self.table.lookup("build.target") {
Ok(Some(v.as_str()
Expand All @@ -167,13 +172,26 @@ impl Config {
Ok(None)
}
}

pub fn target_dir(&self) -> Result<Option<&str>> {
if let Some(v) = self.table.lookup("build.target-dir") {
Ok(Some(
v.as_str().ok_or_else(
|| format!(".cargo/config: build.target-dir must be a string"),
)?,
))
} else {
Ok(None)
}
}
}

pub fn config() -> Result<Option<Config>> {
let cd = env::current_dir().chain_err(|| "couldn't get the current directory")?;

if let Some(p) = util::search(&cd, ".cargo/config") {
Ok(Some(Config {
root: p.to_path_buf(),
table: util::parse(&p.join(".cargo/config"))?,
}))
} else {
Expand Down
1 change: 1 addition & 0 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ fn run() -> Result<ExitStatus> {
&root,
&rustflags,
&meta,
config.as_ref(),
&src,
&sysroot,
verbose,
Expand Down
19 changes: 14 additions & 5 deletions src/sysroot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ fn build(
ctoml: &cargo::Toml,
home: &Home,
rustflags: &Rustflags,
config: Option<&cargo::Config>,
sysroot: &Sysroot,
hash: u64,
verbose: bool,
Expand All @@ -45,6 +46,11 @@ name = "sysroot"
version = "0.0.0"
"#;

let target_dir = match config {
Some(c) => c.target_dir()?,
None => None,
};

let rustlib = home.lock_rw(cmode.triple())?;
rustlib
.remove_siblings()
Expand Down Expand Up @@ -153,12 +159,13 @@ version = "0.0.0"
}

// Copy artifacts to Xargo sysroot
let base_dir = match target_dir {
Some(dir) => config.unwrap().root().join(dir),
Copy link
Collaborator

@RalfJung RalfJung Sep 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you fold the two new matches into one here, the code becomes shorter and you avoid an unwrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because I want to do td.join("target") if config is None or config.target_dir()? is None. So the only way I can think of to avoid the unwrap is to have nested matches and have td.join("target") twice. Not sure if that's better.

Copy link
Collaborator

@RalfJung RalfJung Sep 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I see.
Either way please move let target_dir down to the other dir stuff here.

But then, wouldn't this work?

let target_dir = match config {
  Some(c) => c.target_dir()?.map(|d| c.root().join(d)),
  None => None,
};
let target_dir = target_dir.unwrap_or_else(|| td.join("target"));

That's equivalent to your code, right?
However, it makes me wonder if this is really what we want. This directory will be outside the tempdir, right? We'll be spilling files into the user's dir and they might get mixed up with other stuff? Shouldn't this be more like td.join(target_dir) or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good idea. Yes, that's equivalent and better, I'll use that.

From my point of view it's the desired behavior. Without setting the target-dir, the files are put into a temp directory. With the target-dir, the files are put into that directory, relative to the root directory of the project. So for example, you can set it to "build/sysroot" and then all files go into "<yourproject>/build/sysroot".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is target-dir usually used for? Isn't that to change target to something else? So as a user I would expect by projects stuff to be put there, but not the sysroot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I created the patch quite some time ago, but I remember that without target-dir, Xargo was putting all kinds of things in the usual target directory. I just tested it again and actually, using a temp directory for the sysroot and using CARGO_TARGET_DIR to store the project stuff at a different place, looks fine.

Maybe Xargo behaves differently now or I'm missing something. However, I would suggest that we close the PR for now and I report back if there still is a good reason for target-dir.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right. Thanks a lot for looking into this!

None => td.join("target"),
};
util::cp_r(
&td.join("target")
.join(cmode.triple())
.join(profile())
.join("deps"),
&dst,
&base_dir.join(cmode.triple()).join(profile()).join("deps"),
&dst
)?;
}

Expand Down Expand Up @@ -221,6 +228,7 @@ pub fn update(
root: &Root,
rustflags: &Rustflags,
meta: &VersionMeta,
config: Option<&cargo::Config>,
src: &Src,
sysroot: &Sysroot,
verbose: bool,
Expand All @@ -239,6 +247,7 @@ pub fn update(
&ctoml,
home,
rustflags,
config,
sysroot,
hash,
verbose,
Expand Down