Create initial prototype of dotnet-user-secrets with MSBuild support #178

natemcmaster · 2016-09-28T22:10:16Z

A prototype port of "dotnet-user-secrets" from project.json to MSBuild support.

This CLI project tool requires the ability to evaluate the project file to find the 'UserSecretsId' property. At the moment, this change will introduce a dependency on the existence and location of the .NET Core SDK. Without this, we cannot properly set the "MSBuildExtensionsPath" property which leads to project evaluation failing on initialization.

NB: merging to feature/msbuild, not dev. More refinement needed before this is ready for a nightly version.

TODO:

Can we remove dependency on existence of MSBuild inside .NET Core SDK? (I hope so.) Not for now. Maybe for 1.0.0 we can.
How to publish this thing as a nupkg? (see Cannot use MSBuild API from a dotnet-CLI project tool dotnet/msbuild#1097 and Microsoft.Build.Runtime is not a complete build environment dotnet/msbuild#1096) Punting this for now. Will address when CLI stabilizes.
Better error handling project evaluation fails to find SDK
Provide upgrade path from project.json to msbuild - see Add migration rule for 'userSecretsId' dotnet/cli#4326
Update Microsoft.Extensions.Configuration.UserSecrets to support MSBuild scenarios - Add support for assembly attribute containing user secret id Configuration#525

cc @NTaylorMullen @bricelam @prafullbhosale @davidfowl @jeffkl @eerhardt

jeffkl · 2016-09-28T22:13:59Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectReader.cs

+            var sdk = sdkResolver.ResolveProjectSdk(projectDir);
+
+            // workaround limitations in using MSBuild API
+            Environment.SetEnvironmentVariable("MSBUILD_EXE_PATH", Path.Combine(sdk.BasePath, "MSBuild.exe"));


This is interesting it looks like you're reusing the MSBuild that ships with the dotnet CLI is that correct?

Yes, that's the only way I've been able to get this working.... :-/

Is this running as one of these "project tools" where it's not deployed via dotnet publish then?

Or do you get errors about missing .props or .targets files?

Yeah, this one of those "project tools". It's deployed into a user project by dotnet-cli with that funky ".nuget/packages/.tools/" folder magic.

The error is dotnet/msbuild#1097. I've also attempted to use a version of MSBuild that is shipped with my tool, but have found issues getting all of MSBuild dependencies into one place (https://github.com/Microsoft/msbuild/issues/1096)...which is why for now this prototype finds the MSBuild inside the .NET Core SDK.

Okay yeah we don't have a good story yet for these project tools. I guess this is valid but hopefully this isn't abused.

I'm also working on a change to build MSBuild.dll instead of MSBuild.exe like we should be. This would break this tool if you try and take a newer package and I'm still investigating who else will be broken.

I guess this is valid but hopefully this isn't abused.

IIUC this is exists primarily as a test hook. I'm kind of hoping dotnet/msbuild#1097 results in some API that we can use to tell MSBuild "here's the runtime you should use".

jeffkl · 2016-09-28T22:29:12Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectReader.cs

+            var sdk = sdkResolver.ResolveProjectSdk(projectDir);
+
+            // workaround limitations in using MSBuild API
+            Environment.SetEnvironmentVariable("MSBUILD_EXE_PATH", Path.Combine(sdk.BasePath, "MSBuild.exe"));


Okay yeah we don't have a good story yet for these project tools. I guess this is valid but hopefully this isn't abused.

I'm also working on a change to build MSBuild.dll instead of MSBuild.exe like we should be. This would break this tool if you try and take a newer package and I'm still investigating who else will be broken.

jeffkl · 2016-09-28T22:29:51Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectReader.cs

+                project = new Project(xml, globalProperties, /* toolsVersion: */ null, projectCollection);
+            }
+
+            var userSecretsId = project.Properties.FirstOrDefault(p => p.Name == "UserSecretsId")?.EvaluatedValue;


Is there a reason you're not using GetPropertyValue?
https://msdn.microsoft.com/en-us/library/microsoft.build.evaluation.project.getpropertyvalue.aspx

I assumed it would throw if the property didn't exist, but the API doc says otherwise, so I'll update to use that API.

jeffkl · 2016-09-28T22:34:39Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectReader.cs

+{
+    internal class MsBuildProjectReader
+    {
+        public string ReadUserSecretId(IFileInfo fileInfo)


If I wrote this class, I would have it's constructor take IFileInfo, add a private method like GetPropertyValue(string name), and change ReadUserSecretId() to call it passing "UserSecretsId". This way you could add new methods to read new properties. But I have no idea the future of this tool so do what you want.

Yeah...i'm kind of intentionally trying to make this class useless to others at the moment. I don't want others to copy-paste without understanding its implementation....plus I'm not sure the implementation is good...still learning which MSBuild APIs are appropriate to use.

But once I'm more confident in my MSBuild-fu, I'll make changes as you suggested and will likely introduce a implementation that can be shared with all aspnet tools that need to evaluate projects.

troydai

A few things to address.

public types in internal namespace;
a few more empty lines to improve readability.

troydai · 2016-09-29T03:00:40Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdk.cs

+
+namespace Microsoft.Extensions.ProjectModel.Internal
+{
+    internal class DotNetCoreSdk


as far as i remember, we keep the types in *.Internal public. it is to avoid over-internalized types but explicitly states that types in *.Internal namespace are subject to change.

troydai · 2016-09-29T03:00:55Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+
+namespace Microsoft.Extensions.ProjectModel.Internal
+{
+    internal class DotNetCoreSdkResolver


same as above.

troydai · 2016-09-29T03:01:18Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+
+        public DotNetCoreSdkResolver(string installationDir)
+        {
+            _installationPath = installationDir;


why not keep the names consistent?

troydai · 2016-09-29T03:08:14Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+
+        public DotNetCoreSdk ResolveProjectSdk(string projectDir)
+        {
+            var global = ResolveGlobalJsonSdkVersion(projectDir);


use a better variable name: sdkVersion

troydai · 2016-09-29T03:09:04Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+        private IEnumerable<string> Installed
+            => Directory.EnumerateDirectories(Path.Combine(_installationPath, "sdk"));
+
+        private string ResolveGlobalJsonSdkVersion(string start)


position private method behind public methods.

troydai · 2016-09-29T03:10:18Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+            }
+        }
+
+        public DotNetCoreSdk ResolveLatest()


can this method be private?

troydai · 2016-09-29T03:30:18Z

src/Microsoft.Extensions.SecretManager.Tools/Program.cs

-            Logger.LogDebug(Resources.Message_Project_File_Path, fileInfo.PhysicalPath);
-            return ReadUserSecretsId(fileInfo);
-        }
+            var physicalFile = new PhysicalFileInfo(fileInfo);


create this instance after test if fileInfo exists.

victorhurdugaci · 2016-09-30T16:45:10Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectReader.cs

+{
+    internal class MsBuildProjectReader
+    {
+        public string ReadUserSecretId(IFileInfo fileInfo)


Do you need an IFileInfo or could you just use Stream instead (and maybe keep this overload too)? It makes testing easier

IFileInfo helps also capture info about the directory which we need to ensure we get the right SDK version.

victorhurdugaci · 2016-09-30T16:47:17Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectReader.cs

+            var sdk = sdkResolver.ResolveProjectSdk(projectDir);
+
+            // workaround limitations in using MSBuild API
+            Environment.SetEnvironmentVariable("MSBUILD_EXE_PATH", Path.Combine(sdk.BasePath, "MSBuild.exe"));


Just a note here in case you plan to use this code in dotnet watch. When you spin a new child process, it inherits the environment variables. Not sure if this will impact the watched app but it might have some interesting side effects

I don't think it will be too big an issues as dotnet-watch will shell out to the SDK which takes care of ensuring environment variables are set correctly before MSBuild.exe is invoked.

NTaylorMullen

⌚

NTaylorMullen · 2016-09-30T17:28:39Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/CommandLineOptions.cs

@@ -33,6 +34,11 @@ public static CommandLineOptions Parse(string[] args, TextWriter output)
            var optionProject = app.Option("-p|--project <PROJECT>", "Path to project, default is current directory",
                CommandOptionType.SingleValue, inherited: true);

+            // the escape hatch if project evaluation fails, or if users want to alter a secret store other than the one
+            // in the current project
+            var optionId = app.Option("--id", "The user secret id to use.",


When would project evaluation fail? Trying to determine if we really need this.

When would project evaluation fail?

At the moment, the question is "When does project evaluation not fail?"

Bad XML, missing imports, invalid syntax, etc... there are lots of reasons why it's good to have a an escape hatch. Plus, some may prefer to just us --id and avoid the MSBuild complexity.

NTaylorMullen · 2016-09-30T17:30:40Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+        }
+
+        private IEnumerable<string> Installed
+            => Directory.EnumerateDirectories(Path.Combine(_installationPath, "sdk"));


Given the fact that we're already tying this expectation to an SDK folder there's no value in taking in the installationDir. Lets group all of our questionable "where is msbuild.exe" assumptions in this one class.

NTaylorMullen · 2016-09-30T17:31:11Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+            {
+                return null;
+            }
+            var t = new { sdk = new { version = "" } };


Make a private class, this is super hacky. That will enable you to not declare this here. See next coment

NTaylorMullen · 2016-09-30T17:33:10Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+            var t = new { sdk = new { version = "" } };
+            try
+            {
+                var obj = JsonConvert.DeserializeAnonymousType(File.ReadAllText(globalJson.FullName), t);


Break this statement apart. Aka:

var globalJsonContents = File.ReadAlText(globalJson.FullName); var globalJson =JsonConvert.Deserialize<GlobalJson>(globalJsonContents);

NTaylorMullen · 2016-09-30T17:33:39Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+        {
+            var first = Installed.Select(d => new { path = d, version = SemanticVersion.Parse(Path.GetFileName(d)) })
+                .OrderByDescending(sdk => sdk.version)
+                .First();


Do we want to handle cases when SDK's haven't been installed?

In theory, it's not possible for "dotnet user-secrets" to ever be invoked without some SDK installed...but at the very least we need a better error message for that.

NTaylorMullen · 2016-09-30T17:36:31Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/DotNetCoreSdkResolver.cs

+                };
+            }
+
+            return ResolveLatest();


Odd that above you're newing up a DotNetCoreSDK in the case that you can resolve a globaljson sdk but not in this case. It's inconsistent.

If you're going to have the ResolveLatest/ResolveGlobalJsonSdkVersion methods separate you should normalize their output.

NTaylorMullen · 2016-09-30T17:38:22Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectReader.cs

+
+namespace Microsoft.Extensions.SecretManager.Tools.Internal
+{
+    internal class MsBuildProjectReader


Doesn't look like this class has any state. Any value in it not being static?

NTaylorMullen · 2016-09-30T17:40:24Z

src/Microsoft.Extensions.SecretManager.Tools/Program.cs

            {
-                projectPath = Path.Combine(projectPath, "project.json");
+                var projects = directoryInfo.EnumerateFileSystemInfos("*.csproj").ToList();


We should normalize all this logic across our "complex" tools. Also, your repo where you normalize project.json and MSBuild project reading seem like the correct approach to move forward. Aka, our tools need to continue supporting both project.json and MSBuild for a little while, might as well make the process painless for all 😄

Yeah, I'm all for recreating some kind of "ProjectResolver" that finds either project.json or msbuild files.

natemcmaster · 2016-10-03T19:00:33Z

🆙 📅 responded to much of the feedback above. Refactored MSBuild evaluation into reusable components.

NTaylorMullen

Prototype seems alright, will get cleaned up even more when project model abstractions comes through.

NTaylorMullen · 2016-10-03T19:12:46Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectContextFactory.cs

+
+        public MsBuildProjectContextFactory()
+        {
+            var dotNetHome = Path.GetDirectoryName(new Muxer().MuxerPath);


Why not group this logic in the DotNetCoreSdkResolver?

In #180 I have moved this into an abstraction that represents MsBuildContext

natemcmaster · 2016-10-05T19:17:18Z

🆙 📅 rebased on top of project model abstractions.

natemcmaster · 2016-10-05T19:17:46Z

src/Microsoft.Extensions.SecretManager.Tools/Internal/MsBuildProjectFinder.cs

+
+namespace Microsoft.Extensions.SecretManager.Tools
+{
+    internal class MsBuildProjectFinder


Would this be useful in project model abstractions too?

Yes. If you put it into PR i'll review it quickly 😄

@NTaylorMullen added to #181

NTaylorMullen · 2016-10-05T22:01:26Z

Any reason to not support both project.json and msbuild?

natemcmaster · 2016-10-05T22:41:59Z

@NTaylorMullen I would assume we won't want to support both PJ and MSBuild, but I've opened that up for discussion with our PMs.

natemcmaster · 2016-10-06T21:27:04Z

Ping @NTaylorMullen any other changes needed before I merge to feature/msbuild ?

NTaylorMullen

Few unimportant questions but overall

NTaylorMullen · 2016-10-06T23:12:31Z

src/Microsoft.Extensions.SecretManager.Tools/Program.cs

@@ -134,59 +136,37 @@ internal int RunInternal(params string[] args)
                CommandOutputProvider.LogLevel = LogLevel.Debug;
            }

-            var userSecretsId = ResolveUserSecretsId(options);
+            var userSecretsId = !string.IsNullOrEmpty(options.Id)


nit: change order for clarity:

var userSecretsId = string.IsNullOrEmpty(options.Id) ? ResolveIdFromProject(options.Project) : options.Id;

NTaylorMullen · 2016-10-06T23:13:24Z

src/Microsoft.Extensions.SecretManager.Tools/Program.cs

+                    .AsDesignTimeBuild()
+                    .WithBuildTargets(Array.Empty<string>())
+                    .WithProjectFile(projectFile)
+                    .WithTargetFramework("") // TFM doesn't matter


Why do we need to set it then? Seems like this should be a default

This tool doesn't need to worry about TFMs. We just want to read a global property. The alternative would be .BuildAllTargetFrameworks().First()

NTaylorMullen · 2016-10-06T23:13:54Z

src/Microsoft.Extensions.SecretManager.Tools/Program.cs

+                    .WithTargetFramework("") // TFM doesn't matter
+                    .Build();
+
+                return project.GetUserSecretsId();


Make debugging easier

var id = project.GetUserSecretsId(); return id;

how does this make debugging easier?

NTaylorMullen · 2016-10-06T23:15:05Z

test/Microsoft.Extensions.ProjectModel.Tests/project.json

-        "type": "build",
-        "version": "1.0.0-*"
-    },
+    "Microsoft.Extensions.ProjectModel.Sources": "1.0.0-*",


Why not build time dependency?

Less typing (shrugs lazily). It's a test project so doesn't matter either way.

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

jeffkl reviewed Sep 28, 2016

View reviewed changes

jeffkl approved these changes Sep 28, 2016

View reviewed changes

troydai reviewed Sep 29, 2016

View reviewed changes

victorhurdugaci reviewed Sep 30, 2016

View reviewed changes

NTaylorMullen suggested changes Sep 30, 2016

View reviewed changes

NTaylorMullen reviewed Oct 3, 2016

View reviewed changes

natemcmaster force-pushed the namc/usersecrets-msbuild branch from 17fe71a to 7179e6b Compare October 5, 2016 19:15

natemcmaster changed the base branch from feature/msbuild to namc/up-msbuild October 5, 2016 19:15

natemcmaster commented Oct 5, 2016

View reviewed changes

natemcmaster force-pushed the namc/usersecrets-msbuild branch from 7179e6b to 5e1fce4 Compare October 5, 2016 20:31

natemcmaster force-pushed the namc/up-msbuild branch from 20a16a4 to dde0229 Compare October 5, 2016 22:17

natemcmaster changed the base branch from namc/up-msbuild to feature/msbuild October 5, 2016 22:18

natemcmaster force-pushed the namc/usersecrets-msbuild branch from 5e1fce4 to f067ae0 Compare October 5, 2016 22:40

natemcmaster mentioned this pull request Oct 6, 2016

Store UserSecretsId in MSBuild project file instead of project.json #169

Closed

Create initial prototype of dotnet-user-secrets with MSBuild support

688a0ab

natemcmaster force-pushed the namc/usersecrets-msbuild branch from f067ae0 to 688a0ab Compare October 6, 2016 21:23

Better error handling of invalid project formats

42fb076

NTaylorMullen approved these changes Oct 6, 2016

View reviewed changes

"type": "build" and minor tweaks from PR feedback.

fc08200

natemcmaster merged commit ef63762 into feature/msbuild Oct 7, 2016

natemcmaster deleted the namc/usersecrets-msbuild branch October 7, 2016 16:56

Create initial prototype of dotnet-user-secrets with MSBuild support #178

Create initial prototype of dotnet-user-secrets with MSBuild support #178

Conversation

natemcmaster commented Sep 28, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troydai 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

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

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

NTaylorMullen 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

NTaylorMullen Sep 30, 2016 • edited Loading

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

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

natemcmaster commented Oct 3, 2016

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen commented Oct 5, 2016

natemcmaster commented Oct 5, 2016

natemcmaster commented Oct 6, 2016

NTaylorMullen 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

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

natemcmaster commented Sep 28, 2016 •

edited

Loading

NTaylorMullen Sep 30, 2016 •

edited

Loading