Uh oh!
There was an error while loading. Please reload this page.
Use std::process::Command throughout compile-test - #43798
Conversation
| procenv: Vec<(String, String)> , | ||
| mut command: Command, | ||
| lib_path: &str, | ||
| aux_path: Option<&str>, |
There was a problem hiding this comment.
I wonder if maybe we could remove these arguments as well? It looks like not all tests need to pass this so we could have a helper which works with these variables and otherwise these aren't passed and the helper isn't called when not needed.
| fn compose_and_run(&self, | ||
| ProcArgs{ args, prog }: ProcArgs, | ||
| procenv: Vec<(String, String)> , | ||
| mut command: Command, |
There was a problem hiding this comment.
One thing that may be worth testing is to take &mut Command here instead of Command. I'm not 100% sure which is more ergonomic, though!
| cmdline, | ||
| }; | ||
| // FIXME (#9639): This needs to handle non-utf8 paths |
There was a problem hiding this comment.
Great to see all these FIXME annotations removed!
alexcrichton
commented
Aug 11, 2017
This looks great to me, thanks! We can always delegate future refactorings to later PRs, this seems great enough to land in the meantime. |
euclio
commented
Aug 11, 2017
Great. Happy to address the comments in the future. |
alexcrichton
commented
Aug 11, 2017
Oh so to be clear, @euclio mind trying to remove the |
euclio
commented
Aug 11, 2017
Ah, sorry for the misunderstanding. I'll see if I can find the time this weekend. |
euclio
commented
Aug 14, 2017
Which invocations actually need the library path munging? Just |
alexcrichton
commented
Aug 14, 2017
Oh I figured we'd just preserve the existing behavior which is that wherever |
bors
commented
Aug 16, 2017
☔ The latest upstream changes (presumably #43710) made this pull request unmergeable. Please resolve the merge conflicts. |
euclio
commented
Aug 16, 2017
@alexcrichton Ok, do the commands ever need just |
euclio
commented
Aug 16, 2017
In other words I'm wondering if there are any invocations that have a None aux path and still need their path munged to include an empty path. |
alexcrichton
commented
Aug 16, 2017
@euclio did you mean to close this? I don't think anything needs If this takes awhile to do though I don't want to block this! I'm fine landing this ahead of time if you'd like to take some more time to game out that change. |
euclio
commented
Aug 16, 2017
Ah, whoops! Reopened. So, I think there's still a little question on how to handle the |
euclio
commented
Aug 16, 2017
Ugh, looks like my rebase borked the branch. Will reopen shortly. |
alexcrichton
commented
Aug 16, 2017
Ok sounds good to me! Feel free to cc me on the new PR and I'll r+ |
Use std::process::Command throughout compile-test Resubmission of #43798. Fixes#43762. r? @alexcrichton
Fixes#43762.
r? @alexcrichton
Fixed up what didn't require a major refactor. I'd like to remove
ProcArgs, but there's no way to get the current program name and args from aCommandbuilder.compose_and_run? I wonder if we could simplify that.expects in the output. Maybe it's worth threading the errors up through.