Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix AltDirectorySeparatorChar on Unix#1842

Closed
BrennanConroy wants to merge 1 commit intodotnet:masterfrom
BrennanConroy:coreCLR-issue-1026-1
Closed

Fix AltDirectorySeparatorChar on Unix#1842
BrennanConroy wants to merge 1 commit intodotnet:masterfrom
BrennanConroy:coreCLR-issue-1026-1

Conversation

@BrennanConroy
Copy link
Copy Markdown
Member

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment is out of date, and incorrect. Probably related to the ':' on Mac above. It should be cleaned up at the same time.

@ellismg
Copy link
Copy Markdown
Contributor

ellismg commented May 26, 2015

Can you please squash? /cc @stephentoub

@BrennanConroy
Copy link
Copy Markdown
Member Author

Done

@kangaroo
Copy link
Copy Markdown

My comment was removed when you force pushed. Could you please fix the comment above about ':' being the primary sep on Mac. Thats anciently out of date.

@VSadov
Copy link
Copy Markdown
Member

VSadov commented May 26, 2015

Interestingly this change matches assumptions made in Roslyn compilers, where we treat AltDirectorySeparatorChar to be '/' unconditionally.

https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/FileSystem/PathUtilities.cs

@kangaroo
Copy link
Copy Markdown

@dotnet-bot test this please

@stephentoub
Copy link
Copy Markdown
Member

Note that this Path.cs file isn't currently being used when running on CoreCLR, so this PR doesn't actually fix anything at present... this file isn't being compiled into anything used on CoreCLR, and rather is only used presently by the .NET Native build of System.Runtime.Extensions. System.Runtime.Extensions is a "partial facade," which means that it contains some types, but type forwards to mscorlib for any of the types in the System.Runtime.Extensions contract that aren't included in the project, and Path is one of those (note that Path.cs isn't listed in the .csproj). As such, the Path from mscorlib in the coreclr repo is being used, both on Windows and on Unix. Eventually we would like to switch over to using this Path.cs in corefx, but that requires more work, as when we do so we'd want to reimplement all of the P/Invokes it relies on to target Unix functionality directly rather than going through libcoreclr's Win32-like PAL (there are also some changes that have been made to the Path.cs in mscorlib that need to be ported here as well).

If/when we make this change to the Path.cs in mscorlib, we'll also need to be cognizant of the corefx tests. I think it's possible some tests will start breaking (due to test issues) once Path is updated with this change, and since the CI system in corefx currently picks up the latest successful build of mscorlib from the coreclr build, as soon as a change like this gets checked in to coreclr, any tests in corefx that depended on this functionality will immediately start failing in all corefx builds / PRs / etc. As such, before checking in such a change to coreclr, any such tests (if there are any) would need to be fixed up appropriately in corefx.

@stephentoub
Copy link
Copy Markdown
Member

I submitted a similar fix to coreclr: dotnet/coreclr#1087

Thankfully only one corefx test starts to fail with this, and I've disabled it with: #1905

@stephentoub
Copy link
Copy Markdown
Member

@BrennanConroy, now that #1087 has been merged, I'm going to go ahead and close this PR. When we're ready to switch to using the Path.cs in corefx rather than in coreclr, we'll be sure to bring this change over along with a bunch of others. Thanks!

@stephentoub stephentoub closed this Jun 4, 2015
@BrennanConroy BrennanConroy deleted the coreCLR-issue-1026-1 branch December 8, 2015 19:10
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
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.

7 participants