Skip to content

std: Fix fs::read_link behavior on Windows#24198

Merged
bors merged 1 commit into
rust-lang:masterfrom
alexcrichton:windows-readlink
Apr 13, 2015
Merged

std: Fix fs::read_link behavior on Windows#24198
bors merged 1 commit into
rust-lang:masterfrom
alexcrichton:windows-readlink

Conversation

@alexcrichton

Copy link
Copy Markdown
Member

The current implementation of using GetFinalPathNameByHandle actually reads all
intermediate links instead of just looking at the current link. This commit
alters the behavior of the function to use a different API which correctly reads
only one level of the soft link.

[breaking-change]

The current implementation of using GetFinalPathNameByHandle actually reads all
intermediate links instead of just looking at the current link. This commit
alters the behavior of the function to use a different API which correctly reads
only one level of the soft link.

[breaking-change]
@alexcrichton

Copy link
Copy Markdown
Member Author

r? @aturon

cc @retep998, @andrew-d

@rust-highfive rust-highfive assigned aturon and unassigned huonw Apr 8, 2015
@rust-highfive

Copy link
Copy Markdown
Contributor

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@sfackler

sfackler commented Apr 8, 2015

Copy link
Copy Markdown
Member

Could you add a test for this?

@retep998

retep998 commented Apr 8, 2015

Copy link
Copy Markdown
Contributor

@sfackler That would require having a symbolic link to test it on, which requires administrator privileges to create by default.

@sfackler

sfackler commented Apr 8, 2015

Copy link
Copy Markdown
Member

Ah, bummer.

@retep998

retep998 commented Apr 8, 2015

Copy link
Copy Markdown
Contributor

@sfackler That said, it should be feasible to make the buildbot slaves able to create symbolic links, and have the test attempt to make a symbolic link and read it, and if it fails with insufficient privileges then we can just pass the test anyway.

@andrew-d

andrew-d commented Apr 8, 2015

Copy link
Copy Markdown
Contributor

@sfackler That said, it should be feasible to make the buildbot slaves able to create symbolic links, and have the test attempt to make a symbolic link and read it, and if it fails with insufficient privileges then we can just pass the test anyway.

I think this is pretty reasonable. Other than that, 👍 to the PR!

@alexcrichton

Copy link
Copy Markdown
Member Author

Unfortunately I've found that tests that pass when something is disabled to not work out too well in the past. Eventually we'll reconfigure the bots, forget to enable them to create symlinks, and then all the tests will just sit around for a bit not testing much.

I will say though that I've manually tested that the implementation works for now at least!

@aturon

aturon commented Apr 13, 2015

Copy link
Copy Markdown
Contributor

@bors: r+

Thanks for the fix!

@bors

bors commented Apr 13, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit f3f99fb has been approved by aturon

@bors

bors commented Apr 13, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit f3f99fb with merge f55e66a...

bors added a commit that referenced this pull request Apr 13, 2015
The current implementation of using GetFinalPathNameByHandle actually reads all
intermediate links instead of just looking at the current link. This commit
alters the behavior of the function to use a different API which correctly reads
only one level of the soft link.

[breaking-change]
@bors

bors commented Apr 13, 2015

Copy link
Copy Markdown
Collaborator

@bors bors merged commit f3f99fb into rust-lang:master Apr 13, 2015
@alexcrichton alexcrichton deleted the windows-readlink branch April 30, 2015 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants