Uh oh!
There was an error while loading. Please reload this page.
Rewrite make-win-dist.py in Rust - #41932
Conversation
rust-highfive
commented
May 12, 2017
(rust_highfive has picked a reviewer for you, use r? to override) |
alexcrichton
left a comment
There was a problem hiding this comment.
Awesome looks great to me here, thanks @wesleywiser!
There was a problem hiding this comment.
With access in Rust, it may actually be best to query build.cc(target) to find the path to gcc here.
There was a problem hiding this comment.
Nowadays I think for this you can just use p.exists()
There was a problem hiding this comment.
It looks to me like that function assumes dst already refers to a full path name like "C:\rust\test.dll" instead of just the folder name which is what this function handles. I can change this function to call util::copy instead of fs::copy though.
There was a problem hiding this comment.
libwinpthread is mentioned twice here?
There was a problem hiding this comment.
You can probably use extend here actually to avoid the collect above, dropping a few lines in the process!
wesleywiser
commented
May 15, 2017
Updated |
There was a problem hiding this comment.
I'd prefer that this function take files: &[Path], path: &[PathBuf] since it doesn't need a vector for either parameter. Looks like the files are really just strings, in which case &[str] would also be fine.
There was a problem hiding this comment.
It seems better to unwrap_or_default here, since PATH being empty is a decent, if not good, default.
wesleywiser
commented
May 15, 2017
@Mark-Simulacrum Fixed |
Mark-Simulacrum
commented
May 15, 2017
I'll give @alexcrichton the final say, though. But looks good to me! |
alexcrichton
commented
May 15, 2017
@bors: r+ Looks great to me too, thanks so much again @wesleywiser! |
bors
commented
May 15, 2017
📌 Commit 173f693 has been approved by |
Rewrite make-win-dist.py in Rust Fixesrust-lang#41568
Mark-Simulacrum
commented
May 16, 2017
This seemed to fail dist in rollup: |
Mark-Simulacrum
commented
May 16, 2017
@bors r- |
Mark-Simulacrum
commented
May 20, 2017
@wesleywiser Have you had a chance to take a look at the failure? |
wesleywiser
commented
May 22, 2017
@Mark-Simulacrum I was having some trouble repro'ing it but I think I've got it now. I'll continue looking at it this week. |
wesleywiser
commented
May 23, 2017
@Mark-Simulacrum I think that should fix it |
Mark-Simulacrum
commented
May 23, 2017
@bors r=alexcrichton |
bors
commented
May 23, 2017
📌 Commit 7eebabe has been approved by |
bors
commented
May 23, 2017
⌛ Testing commit 7eebabe with merge ce15fd0... |
bors
commented
May 23, 2017
💔 Test failed - status-appveyor |
wesleywiser
commented
May 23, 2017
|
bors
commented
May 23, 2017
⌛ Testing commit 7eebabe with merge 1ba03d3... |
bors
commented
May 23, 2017
💔 Test failed - status-appveyor |
Mark-Simulacrum
commented
May 23, 2017
@bors retry
|
bors
commented
May 24, 2017
⌛ Testing commit 7eebabe with merge 4b21210... |
bors
commented
May 24, 2017
💔 Test failed - status-travis |
Mark-Simulacrum
commented
May 24, 2017
@bors retry
|
Rewrite make-win-dist.py in Rust Fixesrust-lang#41568
bors
commented
May 24, 2017
⌛ Testing commit 7eebabe with merge 2ded772... |
bors
commented
May 24, 2017
💔 Test failed - status-appveyor |
Mark-Simulacrum
commented
May 24, 2017
Timeouts, timeouts. It takes us 40 minutes before we even run configure right now, this is probably never going to work until appveyor is fixed realistically. @bors retry |
bors
commented
May 25, 2017
Rewrite make-win-dist.py in Rust Fixes#41568
bors
commented
May 25, 2017
☀️ Test successful - status-appveyor, status-travis |
wesleywiser
commented
May 25, 2017
Woohooo! Thanks @Mark-Simulacrum and @alexcrichton |
Fixes#41568