Skip to content

Conversation

@mateoatr
Copy link
Contributor

The SDK uses a backslash as a directory separator for copied satellite assemblies, which causes trouble when generating a single file bundle in Unix, where it's completely valid to use \ in filenames, thus erroneously embedding these files as 'en-US\Something.resource.dll'.

@ghost
Copy link

ghost commented Aug 12, 2020

Tagging subscribers to this area: @swaroop-sridhar, @agocke
See info in area-owners.md if you want to be subscribed.

{
SourcePath = sourcePath;
BundleRelativePath = bundleRelativePath;
SourcePath = NormalizeDirectorySeparator(sourcePath);
Copy link
Member

Choose a reason for hiding this comment

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

Can this be Windows UNC path? If yes, is the normalization going to work well for Windows UNC paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I think this could break paths like \\x\.... We already normalize relative paths in FileEntry, I made the change there, more economic/safe.

@sbomer
Copy link
Member

sbomer commented Aug 12, 2020

I wonder if it would instead make sense to instead do the normalization in the SDK at https://github.com/dotnet/sdk/blob/750e56eaa044ba730be3fc39fc715e6bef38eeb7/src/Tasks/Microsoft.NET.Build.Tasks/GenerateBundle.cs#L54? That way HostModel would not need to work around the inconsistent and platform-unaware path representation by MSBuild, and it could in theory be used with filenames containing \ on unix.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

I'm fine with fixing it up here, if we can get this to work, but the in-depth solution is probably to go back to that original code and stop using \ to separate the paths. We could either use / or some Path.Combine helper if MSBuild provides one

{
Type = fileType;
RelativePath = relativePath.Replace(Path.DirectorySeparatorChar, DirectorySeparatorChar);
RelativePath = relativePath.Replace('\\', DirectorySeparatorChar);
Copy link
Member

Choose a reason for hiding this comment

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

Won't this break if this is a unix path that actually has a \ in the file name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would, but I'm not sure what the experience should be for these cases... dotnet new console -o 'a\b\c' or having a C# file named 'x\y\z.cs' and then doing dotnet build won't work either.

Copy link
Member

Choose a reason for hiding this comment

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

You're right -- it looks like the rest of the tooling doesn't like that either. In that case, I'm fine with this fix. The scenario we'd be breaking really isn't supported today.

@sbomer
Copy link
Member

sbomer commented Aug 12, 2020

I think the technically correct solution would be to use a Path.Combine-like helper from MSBuild that uses the target platform's convention, but the RelativePath metadata is public and people tend to use / and \ interchangeably (see dotnet/sdk#12814 (comment)).

@agocke
Copy link
Member

agocke commented Aug 12, 2020

Sure, but if we try to fix it downstream, how can we tell the difference between a \ used as a directory separator, and \ used as a character in a unix file name?

@agocke
Copy link
Member

agocke commented Aug 12, 2020

Any way to test this in the runtime repo?

@mateoatr
Copy link
Contributor Author

Any way to test this in the runtime repo?

Yes, but first we need this change to be available in the SDK. We basically have to run the tests that we already have in BundleLocalizedApp using the SDK instead of the Bundle API.

@vitek-karas vitek-karas merged commit dfb3022 into dotnet:master Aug 17, 2020
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants