Update BuildEnvironmentHelper to locate .NET Core app directory #1069

jeffkl · 2016-09-22T15:09:01Z

In .NET Core, the AppContext.BaseDirectory can be used to locate the application's dependencies.

Related to #1039

In .NET Core, the AppContext.BaseDirectory can be used to locate the application's dependencies.

rainersigwald · 2016-09-22T21:29:52Z

Can you elaborate on why this helps? I don't think I fully understand the scenario.

jeffkl · 2016-09-22T22:12:31Z

When you execute dotnet run, the .NET Core runtime loads references from NuGet package locations. This means Microsoft.Build.Framework.dll is loaded from one place, while Microsoft.Build.dll is loaded from another.

However, when you do dotnet publish, all of the assemblies are placed in the output folder and loaded form it. In these two cases, AppContext.BaseDirectory is the location of the executing app which would reference Microsoft.Build.Runtime package and get the .props and .targets in their output directory. So this change makes it work that your app's directory is used as the location of MSBuild. Does that make sense?

rainersigwald · 2016-09-22T22:37:35Z

Ahh, yes it does make sense! So with the new package, the toolset files will be in AppContext.BaseDirectory even though the DLLs are loaded from elsewhere.

Can you update the comment for this case to be explicit about that scenario and why loading from that directory is right?

rainersigwald · 2016-09-22T22:39:52Z

src/Shared/BuildEnvironmentHelper.cs

+
+#if !CLR2COMPATIBILITY // Assemblies compiled against anything older than .NET 4.0 won't have a System.AppContext
+                // Try the base directory that the assembly resolver uses to probe for assemblies.
+                () => TryFromFolder(AppContext.BaseDirectory, runningTests, runningInVisualStudio, visualStudioPath)


Now that I see what's up, I think this should be above the current-working-directory path. If someone's redistributing an MSBuild runtime, that should be preferred over something that happens to be in the CWD, just as it would be if we found it next to our MSBuild.exe.

I went back and forth on this too. Technically if I see MSBuild.exe (and the runtime) in the current directory, I sort of expect it to be used. That would make sense for an end user but not necessarily for the app developer. It seems rare that someone would change the current directory to some place that has MSBuild and then run a completely different app...

jeffkl · 2016-09-22T22:40:24Z

Yeah but _only_ if you can help me understand why the test fails... Is it because my change needs to be mocked to not return a valid location?

jeffkl · 2016-09-23T14:06:59Z

I have updated the comment, let me know about the test fix when you have a chance.

rainersigwald · 2016-09-23T14:56:23Z

I think I get the test failure. Inside the test run I have AppContext.BaseDirectory set to C:\src\msbuild\bin\Debug-NetCore\Windows_NT_Deployment_Test\, which is accurate--that's where it's running from. But that folder also contains a full toolset--which your new fallback will find. Then a toolset will be found and the test for "don't find toolsets in condition X" will fail.

You should be able to fix this by adding a new delegate like the ones that are already there to return AppContext.BaseDirectory . . . unless you've overridden it to return null in the test, which you should also do.

rainersigwald · 2016-09-23T15:52:22Z

src/XMakeBuildEngine/UnitTests/BuildEnvironmentHelper_Tests.cs

@@ -105,7 +105,7 @@ public void BuildEnvironmentDetectsVisualStudioByEnvironment()
            {
                Environment.SetEnvironmentVariable("VSINSTALLDIR", env.TempFolderRoot);
                Environment.SetEnvironmentVariable("VisualStudioVersion", "15.0");
-                BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly();
+                BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(getAppContextBaseDirectory: ReturnNull);


Do you need to specify this? Seems like it should be redundant (since it's after the env-var fallback).

I was worried if the order ever changed then the test could start to fail so I put this in here to remove the AppContext.BaseDirectory from the equation for all cases. I can remove it if you'd like

I think I'd rather the tests fail in that case.

Okay I'll update the test

jeffkl · 2016-09-23T16:03:37Z

@dotnet-bot test OSX Build for CoreCLR please

jeffkl · 2016-09-23T18:05:55Z

@AndyGerlicher @rainersigwald do you guys approve?

Update BuildEnvironmentHelper to locate .NET Core app directory

670ad65

In .NET Core, the AppContext.BaseDirectory can be used to locate the application's dependencies.

dnfclas added the cla-already-signed label Sep 22, 2016

Sarabeth-Jaffe-Microsoft added the Needs Review label Sep 22, 2016

jeffkl mentioned this pull request Sep 22, 2016

Create a NuGet package containing the MSBuild runtime #1039

Closed

3 tasks

rainersigwald reviewed Sep 22, 2016

View reviewed changes

Update comment

2f3fe25

rainersigwald reviewed Sep 23, 2016

View reviewed changes

jeffkl force-pushed the BuildEnvironmentHelper branch from 77e9cff to 33cb003 Compare September 23, 2016 15:55

Fix unit tests

a8d9a62

jeffkl force-pushed the BuildEnvironmentHelper branch from 33cb003 to a8d9a62 Compare September 23, 2016 16:45

Change the order of the search paths

10c203d

jeffkl merged commit c09995d into dotnet:xplat Sep 23, 2016

Sarabeth-Jaffe-Microsoft removed the Needs Review label Sep 23, 2016

jeffkl deleted the BuildEnvironmentHelper branch September 23, 2016 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update BuildEnvironmentHelper to locate .NET Core app directory #1069

Update BuildEnvironmentHelper to locate .NET Core app directory #1069

jeffkl commented Sep 22, 2016 •

edited

Loading

rainersigwald commented Sep 22, 2016

jeffkl commented Sep 22, 2016

rainersigwald commented Sep 22, 2016

rainersigwald Sep 22, 2016

jeffkl Sep 22, 2016

jeffkl commented Sep 22, 2016

jeffkl commented Sep 23, 2016

rainersigwald commented Sep 23, 2016

rainersigwald Sep 23, 2016

jeffkl Sep 23, 2016

rainersigwald Sep 23, 2016

jeffkl Sep 23, 2016

jeffkl commented Sep 23, 2016

jeffkl commented Sep 23, 2016

Update BuildEnvironmentHelper to locate .NET Core app directory #1069

Update BuildEnvironmentHelper to locate .NET Core app directory #1069

Conversation

jeffkl commented Sep 22, 2016 • edited Loading

rainersigwald commented Sep 22, 2016

jeffkl commented Sep 22, 2016

rainersigwald commented Sep 22, 2016

rainersigwald Sep 22, 2016

Choose a reason for hiding this comment

jeffkl Sep 22, 2016

Choose a reason for hiding this comment

jeffkl commented Sep 22, 2016

jeffkl commented Sep 23, 2016

rainersigwald commented Sep 23, 2016

rainersigwald Sep 23, 2016

Choose a reason for hiding this comment

jeffkl Sep 23, 2016

Choose a reason for hiding this comment

rainersigwald Sep 23, 2016

Choose a reason for hiding this comment

jeffkl Sep 23, 2016

Choose a reason for hiding this comment

jeffkl commented Sep 23, 2016

jeffkl commented Sep 23, 2016

jeffkl commented Sep 22, 2016 •

edited

Loading