-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
Replace hardcoded forward slash with path::MAIN_SEPARATOR #40620
Conversation
r? @BurntSushi (rust_highfive has picked a reviewer for you, use r? to override) |
@bors r+ |
📌 Commit cb4f536 has been approved by |
@laumann Thanks! |
@BurntSushi Thank you :-) Also came across https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/parser.rs#L5307 - can it be fixed here or is it too late? |
@bors r- |
@laumann Probably not a good idea to add commits after a PR has been r+'d. I think it will rightyfully cause the PR to fail to merge. The new changes look OK. Could you please squash them down to a single commit please? |
@BurntSushi OK, can I fix it here, or should I open a new PR? |
@laumann You can fix it here by squashing your branch locally (probably with a rebase) and then force pushing. |
OK, no problem, thanks :-) |
5e39f5e
to
eca493c
Compare
Rebased. Something went wrong with https://travis-ci.org/rust-lang/rust/jobs/212496260, but I can't really tell what. |
☔ The latest upstream changes (presumably #40346) made this pull request unmergeable. Please resolve the merge conflicts. |
@laumann You need to merge changes from master back into this PR because someone else has made changes since submitting your PR that conflict with your changes. You can do this by checking out your |
eca493c
to
4f0141b
Compare
@BurntSushi Rebased. |
@laumann Thanks! |
@bors r+ |
📌 Commit 4f0141b has been approved by |
I think this is causing a test failure on Windows: https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2460/job/kfyqxp0uxi22y3lk @bors r- |
@frewsxcv You are right - it looks like Why is that test in parse-fail and not compile-fail? |
It seems the issue can be fixed several ways, but which one would be preferred?
and add
But I'm wondering what the preferred approach is... I tried fiddling with
But that trick didn't work :-) |
@BurntSushi Should I prefer to amend the existing commit, or should I just add new ones (to fix the test)? |
@laumann Amend the existing commit. We shouldn't have broken commits. :-) |
4f0141b
to
25130da
Compare
@BurntSushi The build failed, but I can't really tell why - is there some way to try again? |
@laumann I kicked it. |
@BurntSushi Yay! Thanks :-) |
@frewsxcv It should be fixed now, not sure if I need to do anything else. |
@@ -8,6 +8,8 @@ | |||
// option. This file may not be copied, modified, or distributed | |||
// except according to those terms. | |||
|
|||
// ignore-windows |
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.
It doesn't seem OK to ignore this test on Windows.
@alexcrichton What should we do here? This PR is changing a few places to use the native path separator when printing file paths, so I assume this test is failing because the error messages don't compare equal. Is there a way to do conditional testing based on platform?
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 now see that there is another test in this PR, mod_file_not_exist_windows.rs
, which essentially does this. But it's done by exhausting every non-Windows platform. Is that the right way to do this?
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.
@BurntSushi I considered different approaches - this could maybe be turned into a run-make test instead.
I went for this solution, because I looked at src/test/codegen/dllimports/main.rs
(and others) that do windows-specific things by exhausting the other options.
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.
Precedence is good enough for me.
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.
Thanks!
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.
Thank you :-)
Ah, of course, you have to r+ it! I was wondering why it wasn't being tested again...
@bors r+ |
📌 Commit 25130da has been approved by |
🔒 Merge conflict |
@BurntSushi Should I rebase again? |
Yup. Looks like something else got merged that conflicts with this, so you'll need to rebase to resolve conflicts. |
Will do |
25130da
to
b376386
Compare
Seems to be good now. 🤞 |
@bors retry |
@bors: r=BurntSushi |
📌 Commit b376386 has been approved by |
Replace hardcoded forward slash with path::MAIN_SEPARATOR Fixes #40149
☀️ Test successful - status-appveyor, status-travis |
Fixes #40149