-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix spurious Windows errors with switch_features_rerun. #6561
Conversation
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
tests/testsuite/support/mod.rs
Outdated
/// method to run the executable. Each time you call this, | ||
/// use a new name for `dst`. | ||
/// See https://github.com/rust-lang/cargo/issues/5481 | ||
pub fn safely_rename_run(&self, src: &str, dst: &str) -> Execs { |
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.
👨🎨 maybe just called run
and with a name automatically derived from src?
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.
Yea, it's a bit wordy. However, I don't want to infer that this is a normal way of running an executable. I dropped the "safely" bit.
To derive the name, it would need to maintain state somewhere to keep the name unique. Project
isn't generally mutable, so it seemed a bit awkward. So I'm unsure if there's a clean way to do that.
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.
Yea, it's a bit wordy. However, I don't want to infer that this is a normal way of running an executable. I dropped the "safely" bit.
It avoids a pitfall on Windows, under certain circumstances. If there's no penalty hit I wouldn't mind people using it as the normal way of running an executable. WDYT?
To derive the name, it would need to maintain state somewhere to keep the name unique.
Project
isn't generally mutable, so it seemed a bit awkward. So I'm unsure if there's a clean way to do that.
I'd just set up another one of these as a run naming counter:
cargo/tests/testsuite/support/paths.rs
Lines 12 to 14 in 87dd6c2
static NEXT_ID: AtomicUsize = ATOMIC_USIZE_INIT; | |
thread_local!(static TASK_ID: usize = NEXT_ID.fetch_add(1, Ordering::SeqCst)); |
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 would prefer if normal execution goes through cargo run
. It's a somewhat different codepath which has had some issues in the past, so I think it's good for it to be fully exercised. cargo run
is also a bit more convenient to use.
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.
Ah, I see.
@bors: r+ Thanks so much for tracking this down! |
📌 Commit 2a1adb3 has been approved by |
⌛ Testing commit 2a1adb3 with merge 74bac357a142a8563edee44ec7bd2bf9b2fa2740... |
💔 Test failed - checks-travis |
Fix spurious Windows errors with switch_features_rerun. Fix a flaky test on Windows which fails with "Access is denied.". Previous errors: https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/17766698 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18040797 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/20472056 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18746372 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18559776 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/19659571 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18956349 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18183191 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/20334485 https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18554058
☀️ Test successful - checks-travis, status-appveyor |
Update cargo Pull in fix for #57774. 6 commits in ffe65875fd05018599ad07e7389e99050c7915be..907c0febe7045fa02dff2a35c5e36d3bd59ea50d 2019-01-17 23:57:50 +0000 to 2019-01-20 22:31:07 +0000 - Put mtime-on-use behind a feature flag. (rust-lang/cargo#6573) - Fix a typo in the unstable docs (rust-lang/cargo#6569) - Perhaps you meant: foo, bar or foobar (rust-lang/cargo#6550) - Refactor: Create uninstall submodule (rust-lang/cargo#6557) - Fix spurious Windows errors with switch_features_rerun. (rust-lang/cargo#6561) - Stop building on master on Travis. (rust-lang/cargo#6562) r? @Mark-Simulacrum
Fix a flaky test on Windows which fails with "Access is denied.".
Previous errors:
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/17766698
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18040797
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/20472056
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18746372
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18559776
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/19659571
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18956349
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18183191
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/20334485
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18554058