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.

Sign up for GitHub

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

Jump to bottom

Do not collapse slashes #1641

Closed
wants to merge 6 commits into from
Closed

Do not collapse slashes #1641

wants to merge 6 commits into from

Conversation

cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Feb 1, 2017 •
edited
Loading

Resolves #1622

The risk here is that crossplatform build scripts which accidentally worked with double slashes won't work any longer.

@jonsequitur

@cdmihai
MaybeAdjustFilePath should not collapse multiple slashes
827e39d 
This ends up destroying data that contains double slashes, like URLs
Resolves dotnet#1622
@cdmihai
Fix RemoveWithWildcards to work without path massaging
13b6887 
Environment.GetEnvironmentVariable("TEMP") does not add a trailing slash whereas Path.GetTempPath() does.

Without the path massaging done by FileUtilities.MaybeAdjustFilePath, this test fails beause the remove filespec ends up having two forward slashes and the matching items do not have the double slash.
@cdmihai cdmihai mentioned this pull request Feb 1, 2017
Merged
rainersigwald
rainersigwald reviewed Feb 2, 2017
View reviewed changes
src/XMakeBuildEngine/UnitTests/BackEnd/IntrinsicTask_Tests.cs Outdated
@@ -2098,15 +2098,15 @@ public void RemoveWithWildcards()
<Target Name='t'>
<ItemGroup>
<i1 Include='" + files.First() + ";" + files.ElementAt(1) + @";other'/>
<i1 Remove='$(temp)" + Path.DirectorySeparatorChar + @"*.tmp'/>
<i1 Remove='$(temp)*.tmp'/>
Copy link
Member

Choose a reason for hiding this comment

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

Was this required? Does that mean that Exclude="something\$(PropertyThatExpandsToEmpty)\*" will break with this change?

Copy link
Contributor Author

@cdmihai cdmihai Feb 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Probably. I'll run this by dev14 and see what the behaviour was. I'd expect that it fails.

Copy link
Contributor Author

@cdmihai cdmihai Feb 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Mkay, so this is how msbuild behaves now on windows. I tried out dev14 and our xplat branch.

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
        <ItemGroup>
                <I Include="a//b//**" Exclude="a/b/*.foo"/>
                <I2 Include="a/b/**" Exclude="a//b/*.foo"/>
        </ItemGroup>

        <Target Name="Build">
                <ItemGroup>
                        <T Include="a/b/**"/>
                        <T Remove="a//b/*.foo"/>
                        <T Remove="a/b//*.foo"/>
                </ItemGroup>
                <Message Text="I:@(I)" Importance="High"/>
                <Message Text="I2:@(I2)" Importance="High"/>
                <Message Text="T:@(T)" Importance="High"/>
        </Target>
</Project>
D:\projects\tests\projects\play> msbuild
Microsoft (R) Build Engine version 14.0.25420.1
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 2/2/2017 11:41:05 AM.
Project "D:\projects\tests\projects\play\build.proj" on node 1 (default targets).
Build:
  I:a//b//a.cs;a//b//b.foo
  I2:a/b/a.cs;a/b/b.foo
  T:a/b/a.cs;a/b/b.foo
Done Building Project "D:\projects\tests\projects\play\build.proj" (default targets).


Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.02
D:\projects\tests\projects\play> D:\projects\msbuild\bin\Bootstrap\15.0\Bin\MSBuild.exe
Microsoft (R) Build Engine version 15.1.543.24285
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 2/2/2017 11:41:12 AM.
Project "D:\projects\tests\projects\play\build.proj" on node 1 (default targets).
Build:
  I:a//b//a.cs;a//b//b.foo
  I2:a/b/a.cs;a/b/b.foo
  T:a/b/a.cs;a/b/b.foo
Done Building Project "D:\projects\tests\projects\play\build.proj" (default targets).


Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.35
D:\projects\tests\projects\play> D:\projects\msbuild\bin\Bootstrap-NetCore\CoreRun.exe D:\projects\msbuild\bin\Bootstrap
-NetCore\MSBuild.dll
Microsoft (R) Build Engine version 15.1.0.0
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 2/2/2017 11:41:33 AM.
Project "D:\projects\tests\projects\play\build.proj" on node 1 (default targets).
Build:
  I:a//b//a.cs;a//b//b.foo
  I2:a/b/a.cs;a/b/b.foo
  T:a/b/a.cs;a/b/b.foo
Done Building Project "D:\projects\tests\projects\play\build.proj" (default targets).

So for Windows at least the behaviour is the same.

On non-windows xplat msbuild gives you this:

Microsoft (R) Build Engine version 15.2.0.0
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 2/2/17 12:06:30 PM.
Project "/Users/micodoba/Dropbox/build.proj" on node 1 (default targets).
Build:
  I:a/b/a.cs
  I2:a/b/a.cs
  T:a/b/a.cs
Done Building Project "/Users/micodoba/Dropbox/build.proj" (default targets).

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.74

And with this PR it would give you this:

Microsoft (R) Build Engine version 15.1.0.0
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 2/2/17 12:09:37 PM.
Project "/Users/micodoba/Dropbox/build.proj" on node 1 (default targets).
Build:
  I:a//b//a.cs;a//b//b.foo
  I2:a/b/a.cs;a/b/b.foo
  T:a/b/a.cs;a/b/b.foo
Done Building Project "/Users/micodoba/Dropbox/build.proj" (default targets).

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.81

So this PR makes the crossplat behaviour more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll also add unit tests for this

src/XMakeBuildEngine/UnitTests/BackEnd/IntrinsicTask_Tests.cs Outdated
</ItemGroup>
</Target></Project>");
IntrinsicTask task = CreateIntrinsicTask(content);
PropertyDictionary<ProjectPropertyInstance> properties = new PropertyDictionary<ProjectPropertyInstance>();
properties.Set(
ProjectPropertyInstance.Create(
"TEMP",
NativeMethodsShared.IsWindows ? Environment.GetEnvironmentVariable("TEMP") : Path.GetTempPath()));
NativeMethodsShared.IsWindows ? Environment.GetEnvironmentVariable("TEMP") + Path.DirectorySeparatorChar : Path.GetTempPath()));
Copy link
Member

Choose a reason for hiding this comment

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

EnsureTrailingSlash()?

src/XMakeBuildEngine/UnitTests/Evaluation/EvaluatorFileNormalization_Tests.cs Outdated


IDictionary<string, string> globalProperties = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
globalProperties.Add("GP", @"/tmp/a//x\\c;ba://");
Copy link
Member

Choose a reason for hiding this comment

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

Was this sufficient to see the URL error before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

@cdmihai cdmihai changed the base branch from vs15-rtw to xplat February 3, 2017 01:20
@cdmihai cdmihai closed this Apr 26, 2018
@cdmihai cdmihai deleted the doNotCollapseSlashes branch April 26, 2018 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainersigwald rainersigwald rainersigwald left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cdmihai @rainersigwald @msftgits @dnfclas @AndyGerlicher