Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

cli: Workaround UNC path fs::canonicalize return #879

Merged
merged 3 commits into from
May 29, 2018

Conversation

alexheretic
Copy link
Member

@alexheretic alexheretic commented May 25, 2018

Url::parse(&format!("file://{}", path.to_str().unwrap())).expect("Bad file name")
let mut path = canonical.to_str().unwrap();

// workaround for UNC path see https://github.com/rust-lang/rust/issues/42869
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexheretic I know this is a workaround, but do you think it might be a good idea to include a Windows-only test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but "\\?\" UNC just won't work wherever it's from so the windows check seemed redundant extra code. Can't see it coming from any other platform though, I think linking to the issue should allow removal if this ever gets solved in canonicalize

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant a Windows-only unit/regression test for the url func, so we'll know if/when that fails, but I'm not sure if it's even possible to create a reliable test for this use case in the AppVeyor CI 🙁

The comment alone is enough, just wanted to ask if it's a good idea to include such a test in the first place.

Copy link
Member Author

@alexheretic alexheretic May 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, a unit test could work. A better test is harder as the source of the path is std::env::current_dir().

The unit test would pretty much just check that it works though, so maybe not a huge value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added one now, left it running on all platforms as then all devs will keep it compiling (and it's a fast test).

Issue is a Windows one, but the test should pass on all platforms
@Xanewok
Copy link
Member

Xanewok commented May 29, 2018

Sorry for the delay and thanks for adding the test!

@Xanewok Xanewok merged commit 1871ca5 into rust-lang:master May 29, 2018
@alexheretic alexheretic deleted the cmd-unc-path branch May 29, 2018 15:28
bors added a commit that referenced this pull request Feb 5, 2019
…r=Xanewok

Bump tokio-timer from 0.2.9 to 0.2.10

Bumps [tokio-timer](https://github.com/tokio-rs/tokio) from 0.2.9 to 0.2.10.
<details>
<summary>Commits</summary>

- [`a69aca8`](tokio-rs/tokio@a69aca8) Bump tokio-timer v0.2.10 ([#886](https://github-redirect.dependabot.com/tokio-rs/tokio/issues/886))
- [`13c9618`](tokio-rs/tokio@13c9618) tokio-timer: Fix multi reset DelayQueue bug ([#871](https://github-redirect.dependabot.com/tokio-rs/tokio/issues/871))
- [`61d4aa9`](tokio-rs/tokio@61d4aa9) docs: replace `Prepends` with `Appends` ([#882](https://github-redirect.dependabot.com/tokio-rs/tokio/issues/882))
- [`9d6d142`](tokio-rs/tokio@9d6d142) Bump tokio-sync v0.1.1 ([#881](https://github-redirect.dependabot.com/tokio-rs/tokio/issues/881))
- [`95b0eec`](tokio-rs/tokio@95b0eec) sync: bounded channel can not have 0 size ([#879](https://github-redirect.dependabot.com/tokio-rs/tokio/issues/879))
- [`e1a07ce`](tokio-rs/tokio@e1a07ce) threadpool: update crossbeam dependencies ([#874](https://github-redirect.dependabot.com/tokio-rs/tokio/issues/874))
- See full diff in [compare view](tokio-rs/tokio@tokio-timer-0.2.9...tokio-timer-0.2.10)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=tokio-timer&package-manager=cargo&previous-version=0.2.9&new-version=0.2.10)](https://dependabot.com/compatibility-score.html?dependency-name=tokio-timer&package-manager=cargo&previous-version=0.2.9&new-version=0.2.10)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants