Skip to content
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

Remove FILEDosToUnixPathA conversion #78995

Merged
merged 5 commits into from
Dec 2, 2022

Conversation

janvorli
Copy link
Member

All file paths passed to the coreclr PAL are processed by this function to convert backslash characters to forward ones. This is causing problems when users have directories with names containing backslashes. Such problems were recently reported by people.

This change removes the FILEDosToUnixPathA and fixes one PAL test and one coreclr test that was passing backslash separated paths.

All file paths passed to the coreclr PAL are processed by this function
to convert backslash characters to forward ones. This is causing problems
when users have directories with names containing backslashes. Such
problems were recently reported by people.

This change removes the `FILEDosToUnixPathA` and fixes one PAL test and
one coreclr test that was passing backslash separated paths.
@janvorli janvorli added this to the 8.0.0 milestone Nov 29, 2022
@janvorli janvorli self-assigned this Nov 29, 2022
@jkotas
Copy link
Member

jkotas commented Nov 29, 2022

There are a few places in the runtime that have similar problem. Could you please scan and fix them as well?

For example,

if (*p == _T('/') || *p == _T('\\'))

@jkotas jkotas added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Nov 29, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 29, 2022
@ghost
Copy link

ghost commented Nov 29, 2022

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@jkotas
Copy link
Member

jkotas commented Nov 29, 2022

This needs to be tracked as a breaking change.

@AaronRobinsonMSFT
Copy link
Member

@elinor-fung Isn't there an issue that is tracking this behavior?

@elinor-fung
Copy link
Member

There may be others, but #75387 is the one I know of (with some other issues closed as duplicates against it).

@MichalStrehovsky
Copy link
Member

NativeAOT is also emulating this:

// do the Dos/Unix conversion
libraryName = libraryName.Replace('\\', '/');

@janvorli
Copy link
Member Author

janvorli commented Dec 1, 2022

@jkotas, @AaronRobinsonMSFT I have scanned the whole coreclr and nativeaot and modified all places where we were either converting the directory separators, or using both or even using backslashes on Unix too. And I've removed
_wsplitpath_s.

src/coreclr/vm/peimage.cpp Outdated Show resolved Hide resolved
@janvorli janvorli force-pushed the remove-dos-to-unix-path-conversion branch from 15fad03 to 48e4852 Compare December 1, 2022 15:53
Exclude the `foo\\bar` case for non-Windows.
@janvorli
Copy link
Member Author

janvorli commented Dec 1, 2022

The AppHost.Bundle.Tests.BundleExtractToSpecificPath.Bundle_Extraction_To_Relative_Path_Succeeds fails with these changes for this [InlineData("foo\\bar", BundleOptions.BundleNativeBinaries)]. This test case should be Windows specific after this change as it expects the foo and bar to be separate directories. We already have an exclusion check for similar case with forward slash on Windows, so I will mimic that.

@janvorli
Copy link
Member Author

janvorli commented Dec 2, 2022

@jkotas, @AaronRobinsonMSFT the CI is green and I believe I have addressed all of the feedback. I'll merge it in the morning unless you have an additional feedback.

@janvorli janvorli merged commit 5df8e75 into dotnet:main Dec 2, 2022
@janvorli janvorli deleted the remove-dos-to-unix-path-conversion branch December 2, 2022 09:01
@janvorli
Copy link
Member Author

janvorli commented Dec 7, 2022

The breaking change issue is here: dotnet/docs#32906

@janvorli janvorli removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Dec 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants