Add templated probing path to store for developer scenarios #1306

ramarag · 2017-06-05T23:25:16Z

This is a followup of dotnet/core-setup#2539
Fixes #1298

gkhanna79 · 2017-06-05T23:47:17Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ComposeStore.targets

@@ -220,7 +220,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    <PropertyGroup>
      <DefaultComposeDir>$(HOME)</DefaultComposeDir>
      <DefaultComposeDir Condition="'$(OS)' == 'Windows_NT'">$(USERPROFILE)</DefaultComposeDir>
-      <DefaultComposeDir>$([System.IO.Path]::Combine($(DefaultComposeDir), '.dotnet', $(PlatformTarget), 'store'))</DefaultComposeDir>
+      <DefaultComposeDir>$([System.IO.Path]::Combine($(DefaultComposeDir), '.dotnet', 'store'))</DefaultComposeDir>


Where does Store output folder get the architecture now from?

Why are we removing this? I would not have expected this to happen.

Previously userprofile store was in
<user_profile><arch>\store

since we have removed both sdk and shared Fx from user_profile, this location no longer makes sense. I have moved store now under <user_profile>

The tooling adds and and passes this path as an additional probing path for developer scenarios

gkhanna79 · 2017-06-06T00:05:55Z

I see. the arch under store subfolder remains, right? Thanks, Gaurav

-------- Original message -------- From: Rama krishnan Raghupathy <notifications@github.com> Date: 6/5/17 4:57 PM (GMT-08:00) To: dotnet/sdk <sdk@noreply.github.com> Cc: "Gaurav Khanna (CLR)" <Gaurav.Khanna@microsoft.com>, Mention <mention@noreply.github.com> Subject: Re: [dotnet/sdk] Add templated probing path to store for developer scenarios (#1306) @ramarag commented on this pull request.

________________________________ In src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ComposeStore.targets<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F1306%23discussion_r120238087&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7Ce2c34ddaa1094b674f9708d4ac6e9c22%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636323038447869681&sdata=pZ1H6Mw86LmhlywRjmUslY3ucWrzZHmUCQqffJ1MB0g%3D&reserved=0>:

<PropertyGroup> <DefaultComposeDir>$(HOME)</DefaultComposeDir> <DefaultComposeDir Condition="'$(OS)' == 'Windows_NT'">$(USERPROFILE)</DefaultComposeDir> - <DefaultComposeDir>$([System.IO.Path]::Combine($(DefaultComposeDir), '.dotnet', $(PlatformTarget), 'store'))</DefaultComposeDir> + <DefaultComposeDir>$([System.IO.Path]::Combine($(DefaultComposeDir), '.dotnet', 'store'))</DefaultComposeDir> Previously userprofile store was in <user_profile><arch>\store since we have removed both sdk and shared Fx from user_profile, this location no longer makes sense. I have moved store now under <user_profile> The tooling adds and and passes this path as an additional probing path for developer scenarios — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F1306%23discussion_r120238087&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7Ce2c34ddaa1094b674f9708d4ac6e9c22%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636323038447869681&sdata=pZ1H6Mw86LmhlywRjmUslY3ucWrzZHmUCQqffJ1MB0g%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJA3wc6cJd6FXYfcllV5u06R5XpyfKMGks5sBJXhgaJpZM4Nwr0a&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7Ce2c34ddaa1094b674f9708d4ac6e9c22%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636323038447869681&sdata=NskWkwhrjzgaQm%2BylxHSPaY0NoH92e7l0xX0zb7BZXw%3D&reserved=0>.

ramarag · 2017-06-06T01:10:42Z

In reply to #1306 (comment)
Yes

ramarag · 2017-06-06T01:23:35Z

cc @livarcocc @nguerrera

gkhanna79 · 2017-06-06T02:33:13Z

Change lgtm. Eric and Livar, can you ptal? Thanks, Gaurav

…

-------- Original message -------- From: Rama krishnan Raghupathy <notifications@github.com> Date: 6/5/17 6:23 PM (GMT-08:00) To: dotnet/sdk <sdk@noreply.github.com> Cc: "Gaurav Khanna (CLR)" <Gaurav.Khanna@microsoft.com>, Mention <mention@noreply.github.com> Subject: Re: [dotnet/sdk] Add templated probing path to store for developer scenarios (#1306) cc @livarcocc<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flivarcocc&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7C02dfdf559f8240dbc5af08d4ac7aaa23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636323090214827392&sdata=J%2BJKFWdePF%2FZWmu%2B6WB4eMmsYXNtBhYVcpdwrnKI0Gk%3D&reserved=0> @nguerrera<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnguerrera&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7C02dfdf559f8240dbc5af08d4ac7aaa23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636323090214827392&sdata=wDpz0tZl617HU%2FVffDp1VL3JzUVZBROp%2BBCBLLad62E%3D&reserved=0> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F1306%23issuecomment-306354892&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7C02dfdf559f8240dbc5af08d4ac7aaa23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636323090214827392&sdata=M2GDcWWtcAkPglaYW7WtPFKhAg%2B%2FIx6YZlDPrli8vxc%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJA3wQ8lXIAW7cgEAqelcNNyuIozINe6ks5sBKobgaJpZM4Nwr0a&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7C02dfdf559f8240dbc5af08d4ac7aaa23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636323090214827392&sdata=u2%2Fg9%2FOINV23K7v98KjRpegz3aTQIzda34veEE5A26c%3D&reserved=0>.

gkhanna79

@eerhardt PTAL as well.

eerhardt · 2017-06-06T14:14:06Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildANetCoreApp.cs

@@ -108,6 +109,16 @@ public void It_targets_the_right_framework_depending_on_output_type(bool selfCon
                actualRuntimeFrameworkVersion.Should().Be(expectedRuntimeVersion);
            }

+            string devruntimeConfigFile = Path.Combine(outputDirectory.FullName, testProject.Name + ".runtimeconfig.dev.json");
+            if (File.Exists(devruntimeConfigFile))


This should assert that the file exists, not silently ignore when the file doesn't exist.

eerhardt · 2017-06-06T14:23:03Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateRuntimeConfigurationFiles.cs

+            }
+
+            //Add developer profile store location as the first path to probe
+            var developerProfileStore = DeveloperStoreLocation +  "/.dotnet/store/|arch|/|tfm|";


This feels odd that we are passing part of the path in on a parameter named DeveloperStoreLocation, and then the C# code appends more folders to the path.

Instead, I would expect the whole path to be built in one location (the MSBuild targets), and then passed in. That way if, for whatever reason, someone can modify the whole path, if necessary.

Also, maybe it would be more extensible to have the parameter be "AdditionalProbingPaths" and take a list of paths. Then the MSBuild targets just passes in one, but we can add more later (or someone else can). Thoughts?

eerhardt · 2017-06-06T14:24:29Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.targets

@@ -140,6 +140,11 @@ Copyright (c) .NET Foundation. All rights reserved.
          Inputs="$(ProjectAssetsFile);$(UserRuntimeConfig)"
          Outputs="$(ProjectRuntimeConfigFilePath);$(ProjectRuntimeConfigDevFilePath)">

+    <PropertyGroup>
+      <DeveloperStoreLocation>$(HOME)</DeveloperStoreLocation>


Does it make sense to reuse DefaultComposeDir here? Instead of duplicating this location between here and ComposeStore.targets.

gkhanna79 · 2017-06-06T17:46:08Z

@ramarag Can you get the feedback incorporated so that we can get the PR ready for approval?

eerhardt · 2017-06-06T18:38:37Z

If necessary, I can make the updates if @ramarag doesn't have the time.

gkhanna79 · 2017-06-06T18:51:29Z

@ramarag Will you be able to get the updates done by 1pm today? If not, @eerhardt it will be great if you can get them going.

gkhanna79 · 2017-06-06T20:03:33Z

@eerhardt Will you be able to get the changes for this?

eerhardt · 2017-06-06T20:04:21Z

Just started working on them now.

bleroy · 2017-06-06T20:58:10Z

Does this need to be reflected in documentation?

eerhardt · 2017-06-06T21:19:12Z

Does this need to be reflected in documentation?

Do we have the multi-level lookup locations listed in the documentation? If so, yes. By default, the host only knows how to look next to itself and in the "global" (ProgramFiles) location for 'runtime stores'.

We removed the %USERPROFILE% location from the host and only add that location when you dotnet run or F5 a project. That location is not used in published applications.

GenerateRuntimeConfigurationFiles should take the list of additional probling paths. Use a common property for the user profile store. Test should assert the runtimeconfig.dev.json file is created.

bleroy · 2017-06-06T21:46:03Z

Do we have the multi-level lookup locations listed in the documentation?

Currently, we don't, so I'll leave it that way, at least for now. Thanks.

gkhanna79 · 2017-06-06T21:59:56Z

Currently, we don't, so I'll leave it that way, at least for now.

See Proposed Changes section at https://github.com/dotnet/core-setup/blob/master/Documentation/design-docs/multilevel-sharedfx-lookup.md - they are documented.

bleroy · 2017-06-06T22:02:18Z

Thanks, this is great. I meant on the docs.microsoft.com topic.

ramarag · 2017-06-06T22:35:03Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateRuntimeConfigurationFiles.cs

@@ -43,6 +43,8 @@ public class GenerateRuntimeConfigurationFiles : TaskBase

        public ITaskItem[] HostConfigurationOptions { get; set; }

+        public ITaskItem[] AdditionalProbingPaths { get; set; }


I think you can use string[] here instead

I have a slight preference to keeping ITaskItem in case we ever want to add metadata to these items.

ramarag · 2017-06-06T22:35:28Z

LGTM

gkhanna79 · 2017-06-07T06:15:27Z

@eerhardt @nguerrera @livarcocc What are the next steps to get this merged?

eerhardt · 2017-06-07T15:40:13Z

@MattGertz for approval

Customer scenario

With the introduction of the dotnet store feature, we first allowed the host to look in %USERPROFILE%\.dotnet by default all the time. This way a developer could run dotnet store for a few dependencies, and then dotnet run their app, and the crossgen'd dependencies would be picked up from %USERPROFILE%.
We have since removed that location from the host because in production scenarios, we don't want it probing %USERPROFILE%.
This change adds the %USERPROFILE% location to be probed only at "dev-time" scenarios like dotnet run by adding it to the runtimeconfig.dev.json file, which isn't used in published apps.

Bugs this fixes:

#1298

Workarounds, if any

Developers would have to manually pass the %USERPROFILE%\.dotnet\store location on the command line every time they wanted to use a runtime store created by dotnet store.

Risk

Low - this re-enables the behavior that the host used to have, but only in dev time scenarios.

Performance impact

One new location is probed during dotnet run.
Developers can use dotnet store to crossgen their dependencies and get their projects executed faster using dotnet run.

Is this a regression from a previous update?

This behavior was consciously removed from the host and added to the SDK.

Root cause analysis:

This behavior was flagged as part of a security push for the new multi-level lookup feature.

How was the bug found?

Design/security review.

MattGertz · 2017-06-07T15:53:48Z

I agree & will take to JoC today.

livarcocc · 2017-06-07T18:55:55Z

@MattGertz for approval. This is not a in-box change Matt.

dotnet/sdk#1306 brought in templated probing paths into the runtime.config.json. These are breaking dotnet test because we assume the paths are valid. Skipping any invalid paths by catching exceptions from Path.Combine. Fixes microsoft#847

dotnet/sdk#1306 brought in templated probing paths into the runtime.config.json. These are breaking dotnet test because we assume the paths are valid. Skipping any invalid paths by catching exceptions from Path.Combine. Fixes #847

…0200221.13 (dotnet#1306) - Microsoft.AspNetCore.Analyzers - 5.0.0-preview.2.20121.13 - Microsoft.AspNetCore.Components.Analyzers - 5.0.0-preview.2.20121.13 - Microsoft.AspNetCore.Mvc.Analyzers - 5.0.0-preview.2.20121.13 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 5.0.0-preview.2.20121.13

dnfclas added the cla-already-signed label Jun 5, 2017

gkhanna79 reviewed Jun 5, 2017

View reviewed changes

Add templated probing path to store for developer scenarios

d844a84

ramarag force-pushed the store_for_f5 branch from 36c4970 to d844a84 Compare June 6, 2017 01:09

gkhanna79 approved these changes Jun 6, 2017

View reviewed changes

eerhardt reviewed Jun 6, 2017

View reviewed changes

Respond to PR feedback.

43758a0

GenerateRuntimeConfigurationFiles should take the list of additional probling paths. Use a common property for the user profile store. Test should assert the runtimeconfig.dev.json file is created.

Merge branch 'release/2.0.0' into store_for_f5

344961a

Remove unused property.

cf3f0b1

ramarag commented Jun 6, 2017

View reviewed changes

nguerrera approved these changes Jun 6, 2017

View reviewed changes

MattGertz added the Pending director approval label Jun 7, 2017

livarcocc added Pending M2 Approval and removed Pending director approval labels Jun 7, 2017

MattGertz added the Approved to merge label Jun 7, 2017

eerhardt merged commit e4fc6f8 into dotnet:release/2.0.0 Jun 7, 2017

eerhardt deleted the store_for_f5 branch June 7, 2017 19:04

eerhardt mentioned this pull request Jun 8, 2017

New templated probing path is breaking dotnet test microsoft/vstest#847

Closed

eerhardt mentioned this pull request Jun 8, 2017

Skip invalid additional probing paths microsoft/vstest#848

Merged

nguerrera mentioned this pull request Jun 10, 2017

Add entry to app.dev.runtimeconfig.json #1298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add templated probing path to store for developer scenarios #1306

Add templated probing path to store for developer scenarios #1306

ramarag commented Jun 5, 2017

gkhanna79 Jun 5, 2017

gkhanna79 Jun 5, 2017

ramarag Jun 5, 2017

gkhanna79 commented Jun 6, 2017 via email

ramarag commented Jun 6, 2017

ramarag commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017 via email

gkhanna79 left a comment

eerhardt Jun 6, 2017

eerhardt Jun 6, 2017

eerhardt Jun 6, 2017

gkhanna79 commented Jun 6, 2017

eerhardt commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017

eerhardt commented Jun 6, 2017

bleroy commented Jun 6, 2017

eerhardt commented Jun 6, 2017

bleroy commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017

bleroy commented Jun 6, 2017

ramarag Jun 6, 2017

eerhardt Jun 7, 2017

ramarag commented Jun 6, 2017

gkhanna79 commented Jun 7, 2017

eerhardt commented Jun 7, 2017

MattGertz commented Jun 7, 2017

livarcocc commented Jun 7, 2017

		@@ -43,6 +43,8 @@ public class GenerateRuntimeConfigurationFiles : TaskBase

		public ITaskItem[] HostConfigurationOptions { get; set; }

		public ITaskItem[] AdditionalProbingPaths { get; set; }

Add templated probing path to store for developer scenarios #1306

Add templated probing path to store for developer scenarios #1306

Conversation

ramarag commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkhanna79 commented Jun 6, 2017 via email

ramarag commented Jun 6, 2017

ramarag commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017 via email

gkhanna79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkhanna79 commented Jun 6, 2017

eerhardt commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017

eerhardt commented Jun 6, 2017

bleroy commented Jun 6, 2017

eerhardt commented Jun 6, 2017

bleroy commented Jun 6, 2017

gkhanna79 commented Jun 6, 2017

bleroy commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramarag commented Jun 6, 2017

gkhanna79 commented Jun 7, 2017

eerhardt commented Jun 7, 2017

MattGertz commented Jun 7, 2017

livarcocc commented Jun 7, 2017