Added support for target-dir in config file. #177
Conversation
|
☔ The latest upstream changes (presumably #190) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This looks reasonable (and sorry that it took so long to get you any feedback). Are you still interested in landing this? |
src/sysroot.rs
Outdated
| None => util::cp_r( | ||
| &td.join("target").join(cmode.triple()).join(profile()).join("deps"), | ||
| &dst | ||
| ), |
There was a problem hiding this comment.
There's a lot of shared code between the two branches here, could you factor that out?
There was a problem hiding this comment.
That's true, I'll improve that.
|
Yes, I will rebase it onto the current master. |
Nils-TUD
force-pushed
the
target-dir
branch
from
1f6ba25 to
941b5b7
Compare
| @@ -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), | |||
There was a problem hiding this comment.
If you fold the two new matches into one here, the code becomes shorter and you avoid an unwrap.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
All right. Thanks a lot for looking into this!
So far, xargo ignored the target-dir key in the config file. This caused the copy of the artifacts to the sysroot to fail if this key was used, because xargo expected them in the wrong place. This patch fixes this problem.