Fix for MSBuild command parser confused by Fully Qualified Path to proj files in Linux root #1894 #1954

coderpatros · 2017-04-07T13:10:05Z

The FileUtilities.LooksLikeLinuxPath() only returns true if the first path
segment is a directory. This can't be true for files in the root directory
on *nix systems. This commit adds an additional check to determine if the
parameter is a file.

coderpatros · 2017-04-07T13:38:36Z

@dotnet-bot test OSX Build for CoreCLR please

coderpatros · 2017-04-07T14:00:39Z

@Sarabeth-Jaffe-Microsoft any chance someone can look at the OSX build failure?

I'm getting this from the console output of the build...

06:41:47 Run condition [Current build status] enabling prebuild for step [[Archive the artifacts]]
06:41:48 [_OSX_CoreCLR_prtest] $ /bin/sh -xe /var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/hudson3370030304074114963.sh
06:41:48 + ./cibuild.sh --scope Test --target CoreCLR
06:41:48 Installing dotnet cli...
06:54:06 curl: (56) SSLRead() return error -9806
06:54:06 ./sdk/1.0.0-preview3-003686/Microsoft.ApplicationInsights.dll: (Empty error message)
06:54:06 tar: Error exit delayed from previous errors.
06:54:06 Restoring BuildTools version 1.0.27-prerelease-00927-05...
06:54:06 Failed to load , error: dlopen(, 1): no suitable image found.  Did find:
06:54:06 	/usr/local/lib/: not a file
06:54:06 	/usr/lib/: not a file
06:54:06 ERROR: Could not restore build tools correctly. See '/Users/dotnet-bot/j/workspace/Microsoft_msbuild/master/_OSX_CoreCLR_prtest/init-tools.log' for more details.1
06:54:06 Initializing BuildTools...
06:54:06 /Users/dotnet-bot/j/workspace/Microsoft_msbuild/master/_OSX_CoreCLR_prtest/init-tools.sh: line 122: /Users/dotnet-bot/j/workspace/Microsoft_msbuild/master/_OSX_CoreCLR_prtest/packages/Microsoft.DotNet.BuildTools/1.0.27-prerelease-00927-05/lib/init-tools.sh: No such file or directory
06:54:06 ERROR: An error occured when trying to initialize the tools. Please check '/Users/dotnet-bot/j/workspace/Microsoft_msbuild/master/_OSX_CoreCLR_prtest/init-tools.log' for more details.1

I re-triggered the build in case it was a once off.

jeffkl

Thanks for the contribution.

Please create a unit test to verify this fixes the issue.

jeffkl · 2017-04-07T15:01:27Z

@mmitche Our OS X build is failing because of a weird curl SSL issue. Do you know if anything has changed with the OS X machines lately or have any idea what could cause this?

07:50:25 curl: (56) SSLRead() return error -9806
07:50:25 ./shared/Microsoft.NETCore.App/1.0.1/System.Net.Sockets.dll: (Empty error message)

mmitche · 2017-04-07T15:16:15Z

@jeffkl @patros Depends on what the test is. The lab that the mac machines are in is having severe network issues at the moment. Did you receive an outage notification (Vs Engineering Outage mail)

jeffkl · 2017-04-07T15:22:32Z

Did you receive an outage notification

Apparently I did, sorry I missed that. Since the action that is failing is downloading a .tar, we'll just assume its related to the network issue. Thanks!

mmitche · 2017-04-07T15:25:32Z

@jeffkl Almost certainly. Speeds are extremely low.

coderpatros · 2017-04-08T03:17:15Z

@jeffkl changed tack a bit to make it easier to unit test. Looks like the OSX build servers are working again too.

cdmihai · 2017-04-11T20:49:09Z

src/Shared/FileUtilities.cs

@@ -463,6 +463,10 @@ internal static bool LooksLikeUnixFilePath(string value)
            {
                return true;
            }
+            else if (Directory.Exists(value) || File.Exists(value))


FYI, this code is on the hot path (might get called hundreds of thousands of times on large builds). Though I don't see a way of reducing IO even further.

That's pretty unfortunate, and I think it'd be worth taking a bit of extra care here.

Could the check just be even more explicit for the file-in-root situation?

if (value[0] == '/' && firstSlash == 0 && (Directory.Exists(value) || File.Exists(value))) { return true; }

rainersigwald

Thanks for the patch! I would like to see consideration of the super-explicit check before we decide to go with this approach.

coderpatros · 2017-04-11T22:36:33Z

@rainersigwald @cdmihai it is a terrible check to have to do. I couldn't think of a good way around it though. I've made it more explicit as requested.

rainersigwald

Agreed that this is unpleasant, but I like the impact reduction of this--the new check shouldn't be very expensive even when run a bajillion times in a build, since it'll usually short-circuit out on a nice simple int-zero check.

Thanks!

coderpatros · 2017-04-11T23:03:41Z

Yeah, that check is wayyyy better than my original one

…oj files in Linux root (#1894) The FileUtilities.LooksLikeLinuxPath() only returns true if the first path segment is a directory. This can't be true for files in the root directory on *nix systems. This commit adds an additional check to determine if the parameter is a file.

dnfclas · 2017-04-12T13:32:05Z

@patros, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

coderpatros · 2017-04-12T13:34:59Z

@dotnet-bot test Windows_NT Build for Desktop please

AndyGerlicher

Looks good, thanks!

rainersigwald · 2017-04-13T18:09:52Z

Thanks for the fix, @patros!

coderpatros · 2017-04-13T20:26:08Z

You're welcome!

FileUtilities.LooksLikeUnixPath() only returned true if the first path segment is a directory. This can't be true for files in the root directory on *nix systems. This commit adds an additional check to determine if the parameter is a file or directory in the root. Fixes dotnet#1894.

dnfclas added the cla-not-required label Apr 7, 2017

Sarabeth-Jaffe-Microsoft added the Needs Review label Apr 7, 2017

jeffkl requested changes Apr 7, 2017

View reviewed changes

Sarabeth-Jaffe-Microsoft removed the Needs Review label Apr 7, 2017

jeffkl approved these changes Apr 7, 2017

View reviewed changes

cdmihai reviewed Apr 11, 2017

View reviewed changes

cdmihai approved these changes Apr 11, 2017

View reviewed changes

rainersigwald requested changes Apr 11, 2017

View reviewed changes

rainersigwald approved these changes Apr 11, 2017

View reviewed changes

dnfclas added the cla-signed label Apr 12, 2017

AndyGerlicher approved these changes Apr 13, 2017

View reviewed changes

rainersigwald merged commit 44daad2 into dotnet:master Apr 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for MSBuild command parser confused by Fully Qualified Path to proj files in Linux root #1894 #1954

Fix for MSBuild command parser confused by Fully Qualified Path to proj files in Linux root #1894 #1954

coderpatros commented Apr 7, 2017

coderpatros commented Apr 7, 2017

coderpatros commented Apr 7, 2017

jeffkl left a comment

jeffkl commented Apr 7, 2017

mmitche commented Apr 7, 2017

jeffkl commented Apr 7, 2017

mmitche commented Apr 7, 2017

coderpatros commented Apr 8, 2017

cdmihai Apr 11, 2017

rainersigwald Apr 11, 2017

rainersigwald left a comment

coderpatros commented Apr 11, 2017

rainersigwald left a comment

coderpatros commented Apr 11, 2017

dnfclas commented Apr 12, 2017

coderpatros commented Apr 12, 2017

AndyGerlicher left a comment

rainersigwald commented Apr 13, 2017

coderpatros commented Apr 13, 2017

Fix for MSBuild command parser confused by Fully Qualified Path to proj files in Linux root #1894 #1954

Fix for MSBuild command parser confused by Fully Qualified Path to proj files in Linux root #1894 #1954

Conversation

coderpatros commented Apr 7, 2017

coderpatros commented Apr 7, 2017

coderpatros commented Apr 7, 2017

jeffkl left a comment

Choose a reason for hiding this comment

jeffkl commented Apr 7, 2017

mmitche commented Apr 7, 2017

jeffkl commented Apr 7, 2017

mmitche commented Apr 7, 2017

coderpatros commented Apr 8, 2017

cdmihai Apr 11, 2017

Choose a reason for hiding this comment

rainersigwald Apr 11, 2017

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

coderpatros commented Apr 11, 2017

rainersigwald left a comment

Choose a reason for hiding this comment

coderpatros commented Apr 11, 2017

dnfclas commented Apr 12, 2017

coderpatros commented Apr 12, 2017

AndyGerlicher left a comment

Choose a reason for hiding this comment

rainersigwald commented Apr 13, 2017

coderpatros commented Apr 13, 2017