Extend dotnet run to invoke a run-command-producing Target
#42240
Conversation
baronfel
changed the title
dotnet run to invoke a run-command-producing Target
This reverts commit 0f4a7fd.
baronfel
force-pushed
the
extend-run-with-target
branch
from
fce1998 to
c35ca62
Compare
I put the dependency on the top-level target, but since the Run target is only valid for a single TFM it needs to be on the 'per-framework' inner build target.
baronfel
force-pushed
the
extend-run-with-target
branch
from
8d6ff76 to
5f097a2
Compare
| var oldPath = Directory.GetCurrentDirectory(); | ||
| _basePath = basePath; | ||
| Directory.SetCurrentDirectory(_basePath); | ||
| Telemetry.Telemetry.CurrentSessionId = null; | ||
| try | ||
| { | ||
| action(); | ||
| } | ||
| finally | ||
| { | ||
| Directory.SetCurrentDirectory(oldPath); | ||
| _basePath = null; |
There was a problem hiding this comment.
The intent of this helper is to make the app act like it's in a given path, but certain codeflows were using Directory.GetCurrentDirectory(). That meant that this helper wasn't doing what it was expected to do. This aligns the helper with its intent.
| @@ -129,7 +129,7 @@ public static CliArgument<string> DefaultToCurrentDirectory(this CliArgument<str | |||
| { | |||
| Description = CommonLocalizableStrings.DisableBuildServersOptionDescription | |||
| } | |||
| .ForwardAsMany(_ => new string[] { "-p:UseRazorBuildServer=false", "-p:UseSharedCompilation=false", "/nodeReuse:false" }); | |||
| .ForwardAsMany(_ => ["--property:UseRazorBuildServer=false", "--property:UseSharedCompilation=false", "/nodeReuse:false"]); | |||
There was a problem hiding this comment.
For testing purposes, since we're using the shared Property option, these output properties to forward to MSBuild are all unified to --project for maximal clarity.
| @@ -173,8 +173,8 @@ public static bool DiagOptionPrecedesSubcommand(this string[] args, string subCo | |||
|
|
|||
| internal static string GetCommandLineRuntimeIdentifier(this ParseResult parseResult) | |||
| { | |||
| return parseResult.HasOption(RunCommandParser.RuntimeOption) ? | |||
| parseResult.GetValue(RunCommandParser.RuntimeOption) : | |||
| return parseResult.HasOption(CommonOptions.RuntimeOption) ? | |||
There was a problem hiding this comment.
remove the bespoke options in favor of the common ones
| [InlineData(new string[] { "-p:prop1=true" }, new string[] { "--property:prop1=true" })] | ||
| [InlineData(new string[] { "--property:prop1=true" }, new string[] { "--property:prop1=true" })] | ||
| [InlineData(new string[] { "--property", "prop1=true" }, new string[] { "--property:prop1=true" })] | ||
| [InlineData(new string[] { "-p", "prop1=true" }, new string[] { "--property:prop1=true" })] | ||
| [InlineData(new string[] { "-p", "prop1=true", "-p", "prop2=false" }, new string[] { "--property:prop1=true", "--property:prop2=false" })] | ||
| [InlineData(new string[] { "-p:prop1=true;prop2=false" }, new string[] { "--property:prop1=true", "--property:prop2=false" })] | ||
| [InlineData(new string[] { "-p", "MyProject.csproj", "-p:prop1=true" }, new string[] { "--property:prop1=true" })] | ||
| // The longhand --property option should never be treated as a project | ||
| [InlineData(new string[] { "--property", "MyProject.csproj", "-p:prop1=true" }, new string[] { "-p:MyProject.csproj", "-p:prop1=true" })] | ||
| [InlineData(new string[] { "--disable-build-servers" }, new string[] { "-p:UseRazorBuildServer=false", "-p:UseSharedCompilation=false", "/nodeReuse:false" })] | ||
| [InlineData(new string[] { "--property", "MyProject.csproj", "-p:prop1=true" }, new string[] { "--property:MyProject.csproj", "--property:prop1=true" })] | ||
| [InlineData(new string[] { "--disable-build-servers" }, new string[] { "--property:UseRazorBuildServer=false", "--property:UseSharedCompilation=false", "/nodeReuse:false" })] |
There was a problem hiding this comment.
These expectations all remained the same, with the exception of the 'full' --property form.
| public void MsbuildInvocationIsCorrect(string[] args, string[] expectedArgs) | ||
| { | ||
|
|
||
| string[] constantRestoreArgs = ["-nologo", "-verbosity:quiet"]; |
There was a problem hiding this comment.
Since we're unifying args better now we get these constants that we have to be aware of.
| { | ||
| launchSettingsModel = default; | ||
| if (!UseLaunchProfile) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var buildPathContainer = File.Exists(Project) ? Path.GetDirectoryName(Project) : Project; | ||
| string propsDirectory; | ||
| var launchSettingsPath = TryFindLaunchSettings(ProjectFileFullPath); |
There was a problem hiding this comment.
Pulled out discovery of launch settings for readability as well. The idea is that we try to find the settings, and then try to find a specific profile inside the settings for later application,
| LaunchProfile = launchProfile; | ||
| NoLaunchProfile = noLaunchProfile; | ||
| Args = args; | ||
| RestoreArgs = GetRestoreArguments(restoreArgs); |
There was a problem hiding this comment.
big change here: we precompute the RestoreArgs because we need them in more than one location now. Once for the implicit restore/build, if any, and once for the project evaluation to read the Run arguments. This is a behavior change/bugfix from previous SDKs, because there older SDKs wouldn't apply any properties so evaluation may differ.
| { Constants.MSBuildExtensionsPath, AppContext.BaseDirectory } | ||
| }; | ||
| // TODO for MSBuild usage here: need to sync loggers (primarily binlog) used with this evaluation | ||
| var project = EvaluateProject(ProjectFileFullPath, RestoreArgs); |
There was a problem hiding this comment.
Extracted prior MSBuild evaluation to a method for legibility, then made each of the following 'transformation' steps methods as well so it all flows.
| var userPassedProperties = DeriveUserPassedProperties(restoreArgs); | ||
| if (userPassedProperties is not null) | ||
| { | ||
| foreach (var (key, values) in userPassedProperties) | ||
| { | ||
| globalProperties[key] = string.Join(";", values); | ||
| } | ||
| } |
There was a problem hiding this comment.
Here's that major difference again - we now detect and forward any global properties to the run evaluation. Future work here should probably also pull the loggers (binlog, terminal logger, console logger) and any configuration to apply to the actual target execution as well, so that we can have a consistent experience between build/restore and run argument evaluation.
| } | ||
| else | ||
| { | ||
| throw new GracefulException("boom"); |
There was a problem hiding this comment.
nit: need a localized message here. We also need a way to surface errors from the build here - we need to hook up some kind of logger, and that logger should probably be TL, but we can't easily do this from code. cc @rainersigwald for this gap - maybe we need to make TL a public API?
There was a problem hiding this comment.
You would need to access the current logger, not hook up a new one, right? Since it already built? So would we need to use a LoggingService or a ProjectCollection to retain access to it?
If you have a list of the ILoggers MSBuild used, I don't think MSBuild needs to know the ILogger is a TL to log a message, but I also don't know how we'd log it from there.
There was a problem hiding this comment.
@Forgind part of the problem is that we're in a hybrid mode of using MSBuild here:
- the restore/build commands that
runinherently relies on use MSBuild like a CLI app, so they express opinions about logging in terms of the CLI arguments passed to MSBuild, not anything more structured - this evaluation/target execution for
runuses MSBuild via the API, so if we want to keep things unified in terms of experience we should try to recreate similar logger configurations, but because we've delegated this to MSBuild via CLI arguments it's hard to get that level of consistency
There was a problem hiding this comment.
Rainer blessed the temporary reflection I've used. Short term will will IVT MSBuild so that the CLI can access the TL types, but medium/long term MSbuild should refactor argument parsing to be something that callers can programmatically invoke and get structured outputs for.
baronfel
force-pushed
the
extend-run-with-target
branch
from
5f097a2 to
a01b1a6
Compare
|
Ok, tests should be green now - in both cases the tests were parsing raw stdout but terminal logger was leaving ANSI text on those streams that the expectations couldn't handle. I've scrubbed it now, but there's a gap here where we need a way to script all ANSI text from a console stream. There are libraries for this but nothing immediately usable. |
There was a problem hiding this comment.
I didn't look at everything in depth, but generally this looks good to me.
One thing that is different is that normally when you run a target to get information, the target should return the values with that information. Here the target doesn't return anything and you are just getting the updated property values. I'm not sure if there's a problem with using a different model.
|
@dsplaisted said:
I talked with @tmeschter a bit about this yesterday and did agreed that it would require some work for the project system to use this model - everything they have expects a The concerns are twofold:
|
|
/backport to release/9.0.1xx |
|
Started backporting to release/9.0.1xx: https://github.com/dotnet/sdk/actions/runs/10532093519 |
…target Context: dotnet/sdk#42155 Context: dotnet/sdk#42240 Fixes: dotnet/sdk#31253 The .NET SDK has introduced a new `ComputeRunArguments` MSBuild target that allows you to set `$(RunCommand)` and `$(RunArguments)` in a more dynamic way. So, on Android: * `ComputeRunArguments` depends on `Install`, so the app is deployed, the `<FastDeploy/>` MSBuild target runs, etc. * `$(RunCommand)` is a path to `adb` * `$(RunArguments)` is an `shell am start` command to launch the main activity. The new implementation also allows us to use the `-p` parameter with `dotnet run`, such as: dotnet run -bl -p:AdbTarget=-d This will pass `-d` to `adb`, which allows you to select an attached device if an emulator is running. Previously, we had no way to pass `-p` arguments to `dotnet run`.
Context: dotnet/sdk#31253 Context: dotnet/sdk#42155 Context: dotnet/sdk#42240 The .NET SDK has introduced a new `ComputeRunArguments` MSBuild target that allows you to set `$(RunCommand)` and `$(RunArguments)` in a more dynamic way. So, on Android: * `ComputeRunArguments` depends on `Install`, so the app is deployed, the `<FastDeploy/>` MSBuild target runs, etc. * `$(RunCommand)` is a path to `adb` * `$(RunArguments)` is an `shell am start` command to launch the main activity. The new implementation also allows us to use the `-p` parameter with `dotnet run`, such as: dotnet run -bl -p:AdbTarget=-d This will pass `-d` to `adb`, which allows you to select an attached device if an emulator is running. Previously, we had no way to pass `-p` arguments to `dotnet run`.
Fixes #42155
This PR implements a new extensibility point in
dotnet runhandling that allows other build logic to configure the way the binary launched bydotnet runis discovered and run.See details on the linked issue, but the gist is that if the project implements the new
ComputeRunArgumentstarget then that target will be used to provide overrides for theproperties, which were already used to define how a project should be launched.
Putting this ability behind a Target means that any prerequisites required to launch a project can be discovered and shown to the user.
When target execution fails, we show that to the user via the existing MSBuild Console Logger. Ideally this would be terminal logger but that's internal at the moment.
If we hack it to reflection-load TerminalLogger, it looks much better IMO:
Testing: