-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add basic support for 'dotnet run file.cs' #46915
base: main
Are you sure you want to change the base?
Changes from 6 commits
aea7240
8094d49
9f51756
5a5206f
254eccb
aa913b0
fe91ade
5c873d1
da30f3e
fcd9a92
eb2a1e8
d08a1be
d646bfc
9b80546
4968d9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,192 @@ | ||||||
// Licensed to the .NET Foundation under one or more agreements. | ||||||
// The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
||||||
#nullable enable | ||||||
jjonescz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
using System.Xml; | ||||||
using Microsoft.Build.Construction; | ||||||
using Microsoft.Build.Definition; | ||||||
using Microsoft.Build.Evaluation; | ||||||
using Microsoft.Build.Execution; | ||||||
using Microsoft.Build.Framework; | ||||||
using Microsoft.Build.Logging; | ||||||
using Microsoft.DotNet.Cli.Utils; | ||||||
using Microsoft.DotNet.Tools.Run; | ||||||
|
||||||
namespace Microsoft.DotNet.Tools; | ||||||
|
||||||
/// <summary> | ||||||
/// Used to build a virtual project file in memory to support <c>dotnet run file.cs</c>. | ||||||
/// </summary> | ||||||
internal sealed class VirtualProjectBuildingCommand | ||||||
{ | ||||||
public Dictionary<string, string> GlobalProperties { get; } = new(StringComparer.OrdinalIgnoreCase); | ||||||
public required string EntryPointFileFullPath { get; init; } | ||||||
|
||||||
public int Execute(string[] binaryLoggerArgs, LoggerVerbosity verbosity) | ||||||
{ | ||||||
var binaryLogger = GetBinaryLogger(binaryLoggerArgs); | ||||||
var consoleLogger = new ConsoleLogger(verbosity); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should use new MSBuild API from @MichalPavlik to auto-detect the use of the Console Logger vs the Terminal Logger. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate? I couldn't find such API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the SDK intentionally doesn't use terminal logger now, see: sdk/src/Cli/dotnet/commands/dotnet-run/RunCommand.cs Lines 507 to 508 in 7b44ce4
Nevertheless, I will change this PR to call the same method so when that's fixed, the new logic will use the same logger. Btw, I think it's because the new API didn't flow here yet. I would rather not change that in this PR as it seems orthogonal. |
||||||
Dictionary<string, string?> savedEnvironmentVariables = new(); | ||||||
try | ||||||
{ | ||||||
// Set environment variables. | ||||||
foreach (var (key, value) in MSBuildForwardingAppWithoutLogging.GetMSBuildRequiredEnvironmentVariables()) | ||||||
{ | ||||||
jjonescz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
savedEnvironmentVariables[key] = Environment.GetEnvironmentVariable(key); | ||||||
Environment.SetEnvironmentVariable(key, value); | ||||||
} | ||||||
|
||||||
// Setup MSBuild. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tiny nit:
Suggested change
|
||||||
ReadOnlySpan<ILogger> binaryLoggers = binaryLogger is null ? [] : [binaryLogger]; | ||||||
var projectCollection = new ProjectCollection( | ||||||
GlobalProperties, | ||||||
[.. binaryLoggers, consoleLogger], | ||||||
ToolsetDefinitionLocations.Default); | ||||||
var parameters = new BuildParameters(projectCollection) | ||||||
{ | ||||||
Loggers = projectCollection.Loggers, | ||||||
LogTaskInputs = binaryLogger is not null, | ||||||
}; | ||||||
BuildManager.DefaultBuildManager.BeginBuild(parameters); | ||||||
|
||||||
// Do a restore first (equivalent to MSBuild's "implicit restore", i.e., `/restore`). | ||||||
// See https://github.com/dotnet/msbuild/blob/a1c2e7402ef0abe36bf493e395b04dd2cb1b3540/src/MSBuild/XMake.cs#L1838. | ||||||
var restoreRequest = new BuildRequestData( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of doing any of this, why can't we make a fake project then "call" RunCommand.Execute (with modified parameters) on it? That would make me feel much more comfortable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I wrote that comment, I'd glanced only very briefly at RunCommand, and many of its methods operated on ProjectInstances, so my question was whether you could create a ProjectInstance and pass it to RunCommand. I looked further just now, and though it does a lot of checks with ProjectInstances, it ultimately creates an MSBuildForwardingApp that calls MSBuild's Main method directly, and there's no overload for that that takes in a ProjectInstance. I will caution against worrying too much about performance at this stage. RunCommand does a lot of work that you don't, and I'd rather this be correct than fast. If this is correct, why is RunCommand bulky? Could RunCommand be cut down and perhaps even made more performant if it were to follow a model similar to yours? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I understand, RunCommand only uses ProjectInstances and MSBuild APIs to get the final executable path and RunArguments (users can use that to override how their project actually runs, e.g., what command line arguments are passed to it). This PR reuses all that. For the build part, RunCommand just calls MSBuild's Main method. We cannot reuse that, there is no way to pass XML project file text to MSBuild via its Main method as far as I know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on that? I'm not sure what work RunCommand does that this PR doesn't. Note that this PR just hooks into the "build" stage of RunCommand. The rest (that executes the built DLL) is still the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see:
So maybe just cleanup on ctrl-c and exceptions? For exceptions, I saw that you had the catch (Exception e) bit below; in the SDK, we normally try to get a more precise exception that includes e.Message as part of a GracefulException rather than being the whole thing printed. This isn't a huge thing in my mind, so if you want to defer it for v2, I'm fine with that. |
||||||
CreateProjectInstance(projectCollection, addGlobalProperties: static (globalProperties) => | ||||||
jjonescz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
{ | ||||||
globalProperties["MSBuildRestoreSessionId"] = Guid.NewGuid().ToString("D"); | ||||||
globalProperties["MSBuildIsRestoring"] = bool.TrueString; | ||||||
}), | ||||||
targetsToBuild: ["Restore"], | ||||||
hostServices: null, | ||||||
BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports | BuildRequestDataFlags.FailOnUnresolvedSdk); | ||||||
var restoreResult = BuildManager.DefaultBuildManager.BuildRequest(restoreRequest); | ||||||
if (restoreResult.OverallResult != BuildResultCode.Success) | ||||||
{ | ||||||
return 1; | ||||||
} | ||||||
|
||||||
// Then do a build. | ||||||
var buildRequest = new BuildRequestData( | ||||||
CreateProjectInstance(projectCollection), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we creating two separate project instances? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we need to set additional global properties in the restore vs the build. As far as I understand MSBuild APIs, this is the only way to do it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BuildRequestData has an overload with global properties There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Those ones only take physical file paths though, right? (And they read the XML from the path passed in.) I need to use a BuildRequestData overload that takes the XML data because my project file doesn't exist on disk. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm...I think you're right. You could consider adding a new API for that to MSBuild, since that seems like it would be very easy to implement, but I'd consider that a v3 change not necessary for this PR. |
||||||
targetsToBuild: ["Build"]); | ||||||
var buildResult = BuildManager.DefaultBuildManager.BuildRequest(buildRequest); | ||||||
if (buildResult.OverallResult != BuildResultCode.Success) | ||||||
{ | ||||||
return 1; | ||||||
} | ||||||
|
||||||
BuildManager.DefaultBuildManager.EndBuild(); | ||||||
return 0; | ||||||
} | ||||||
catch (Exception e) | ||||||
{ | ||||||
Console.Error.WriteLine(e.Message); | ||||||
return 1; | ||||||
} | ||||||
finally | ||||||
{ | ||||||
foreach (var (key, value) in savedEnvironmentVariables) | ||||||
{ | ||||||
Environment.SetEnvironmentVariable(key, value); | ||||||
} | ||||||
|
||||||
binaryLogger?.Shutdown(); | ||||||
consoleLogger.Shutdown(); | ||||||
} | ||||||
|
||||||
static ILogger? GetBinaryLogger(string[] args) | ||||||
{ | ||||||
for (int i = args.Length - 1; i >= 0; i--) | ||||||
{ | ||||||
var arg = args[i]; | ||||||
if (RunCommand.IsBinLogArgument(arg)) | ||||||
{ | ||||||
return new BinaryLogger | ||||||
{ | ||||||
Parameters = arg.IndexOf(':') is >= 0 and var index | ||||||
? arg[(index + 1)..] | ||||||
: "msbuild.binlog", | ||||||
}; | ||||||
} | ||||||
} | ||||||
|
||||||
return null; | ||||||
} | ||||||
} | ||||||
|
||||||
public ProjectInstance CreateProjectInstance(ProjectCollection projectCollection) | ||||||
jjonescz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
{ | ||||||
return CreateProjectInstance(projectCollection, addGlobalProperties: null); | ||||||
} | ||||||
|
||||||
private ProjectInstance CreateProjectInstance( | ||||||
ProjectCollection projectCollection, | ||||||
Action<IDictionary<string, string>>? addGlobalProperties = null) | ||||||
jjonescz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
{ | ||||||
var projectRoot = CreateProjectRootElement(projectCollection); | ||||||
|
||||||
var globalProperties = projectCollection.GlobalProperties; | ||||||
if (addGlobalProperties is not null) | ||||||
{ | ||||||
globalProperties = new Dictionary<string, string>(projectCollection.GlobalProperties, StringComparer.OrdinalIgnoreCase); | ||||||
addGlobalProperties(globalProperties); | ||||||
} | ||||||
|
||||||
return ProjectInstance.FromProjectRootElement(projectRoot, new ProjectOptions | ||||||
{ | ||||||
GlobalProperties = globalProperties, | ||||||
}); | ||||||
} | ||||||
|
||||||
private ProjectRootElement CreateProjectRootElement(ProjectCollection projectCollection) | ||||||
{ | ||||||
var projectFileFullPath = Path.ChangeExtension(EntryPointFileFullPath, ".csproj"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this causes us to use the containing directory of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's correct, the "implicit" or "virtual" project conceptually lives at the same level as the executed file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem desirable. If you're specifically trying to build one file, there could easily be bugs (including compile-time bugs) that would prevent you from dotnet run'ning this file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow. Are you suggesting |
||||||
var projectFileText = """ | ||||||
<Project> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why aren't you just specifying Sdk="Microsoft.NET.Sdk" in the Project element? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the target overrides wouldn't have an effect - the Sdk targets would be implicitly imported after the custom targets, i.e., the Sdk targets would overwrite the custom ones, but we want that the other way around. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a comment so that's clear? |
||||||
<Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" /> | ||||||
|
||||||
<PropertyGroup> | ||||||
<OutputType>Exe</OutputType> | ||||||
<TargetFramework>net9.0</TargetFramework> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is in main this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to making this take into account current TF. Please don't resolve things you haven't fixed. I got here and planned to make pretty much exactly this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I addressed this in a similar place in tests, didn't notice this comment was on a different place.
I found such variable in tests, but not in product code. However, in a follow up PR that adds "grow up" support, I'm adding a test that will verify this code matches the
I would actually prefer that, reusing old threads is hard to follow. |
||||||
<ImplicitUsings>enable</ImplicitUsings> | ||||||
<Nullable>enable</Nullable> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced we should force things like nullable. You can consider making it an option, but this is doing something simple, and forcing people to be perfect on this sort of thing will just be annoying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We intend to match the default experience for console projects and present what we consider idiomatic defaults for modern C#, which means nullable is enabled. If people want to disable it, they can use the directive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't find this argument convincing. Who wants nullability? People who work on large, complex C# projects with numerous developers. Who doesn't want nullability? People trying to do something quickly where they don't care about the general case, only their specific case, and people who are trying to learn. In particular, I still remember that when I was learning C#, if I saw an error message, I didn't really look at it; I just looked at my code to try to see what was wrong and added print statements if necessary. That's ok in some languages (albeit very suboptimal for C#), and it doesn't work at all for nullable. This isn't a hill I'm willing to die on, but I do think we should be very conscious of the effect of Nullable in a context like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you are right, but why we have Consider also that nullability is very difficult and error-prone to add later on, it's best to have it from the start. Nullability is also by default just warnings, not errors, and users starting with C# could definitely ignore them. Despite all that, I wouldn't mind not enabling nullability by default for file-based programs, that's completely valid design decision. But it seems like a big shift where we would be saying "nullability is only for advanced users" but I hear that for the first time, my feeling is that we generally recommend users to always use nullability for their new projects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the best reason for enabling nullable in our templates is just to build awareness. I'm mildly concerned that people who look up the nullable warnings they'd get would be told to disable it in their project files and get confused by that—but I hadn't considered that they're warnings by default. To be honest, I'm working on a Java project at the moment, and I've been consistently ignoring some of the warnings the compiler keeps telling me about—but it isn't intended to scale, so it isn't a problem. With that in mind, I think I'd downgrade how much I care about this to only a mild preference for nullable disabled. Since it sounds like others have expressed preferences for it to stay in, I don't think this needs to change. |
||||||
</PropertyGroup> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we enable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think we should keep the current state where unsafe requires a specific build opt in. Overall we want to have less unsafe code in the ecosystem, not more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's true, then why did you change the default template? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made minimal changes to workaround bugs that prevent it building without a physical file on disk. There is a tracking issue linked and ideally that will be fixed and the workaround can be removed and the template will exactly match the default one. |
||||||
|
||||||
<Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" /> | ||||||
|
||||||
<!-- Override targets which don't work with project files that are not present on disk. --> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok now this is some deep magic. It looks like all of this is Restore-time stuff, do we need to add some kinds of flags/detections to enable in-memory use cases for real instead of requiring these workarounds? cc @rainersigwald / @nkolev92 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels strange that an xml string is needed to make an in-memory project, when we are using the msbuild APIs directly for so many other things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Isn't that same with strings containing C# that you need when using Roslyn APIs? You could create SyntaxTrees manually but no one does that. Similarly, I could create the MSBuild project without the string but seems that would be less readable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I am relatively comfortable with it, especially if it remains as "just a string" and we don't start using a complex interpolated string with XML escaping handling, etc. to make the project. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, ideally the targets would allow in-memory builds. Should I file an issue for it at https://github.com/NuGet/Home since the targets live there I think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not quite - the big problem is that a key file in the You probably want to make sure that they synthetic project name is unique-per-app too to account for MSBuild logic that does things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you help me understand why is the scoping of the file name a problem? @jjonescz I think it's a good to consider some sort of a design review for #47020. It involves NuGet gestures, but the NuGet team hasn't really had a chance to look into that yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nkolev92 when I was looking at the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scoping could be a problem if we allow multiple virtual projects in one folder, but the proposal currently says they would get separate artifacts folders, so that should avoid any problems.
I think it is expected that the implementation details of this feature might change over time, so there is no rush. |
||||||
|
||||||
<Target Name="_FilterRestoreGraphProjectInputItems" | ||||||
DependsOnTargets="_LoadRestoreGraphEntryPoints" | ||||||
Returns="@(FilteredRestoreGraphProjectInputItems)"> | ||||||
<ItemGroup> | ||||||
<FilteredRestoreGraphProjectInputItems Include="@(RestoreGraphProjectInputItems)" /> | ||||||
</ItemGroup> | ||||||
</Target> | ||||||
|
||||||
<Target Name="_GetAllRestoreProjectPathItems" | ||||||
DependsOnTargets="_FilterRestoreGraphProjectInputItems" | ||||||
Returns="@(_RestoreProjectPathItems)"> | ||||||
<ItemGroup> | ||||||
<_RestoreProjectPathItems Include="@(FilteredRestoreGraphProjectInputItems)" /> | ||||||
</ItemGroup> | ||||||
</Target> | ||||||
|
||||||
<Target Name="_GenerateRestoreGraph" | ||||||
DependsOnTargets="_FilterRestoreGraphProjectInputItems;_GetAllRestoreProjectPathItems;_GenerateRestoreGraphProjectEntry;_GenerateProjectRestoreGraph" | ||||||
Returns="@(_RestoreGraphEntry)"> | ||||||
<!-- Output from dependency _GenerateRestoreGraphProjectEntry and _GenerateProjectRestoreGraph --> | ||||||
</Target> | ||||||
</Project> | ||||||
"""; | ||||||
ProjectRootElement projectRoot; | ||||||
using (var xmlReader = XmlReader.Create(new StringReader(projectFileText))) | ||||||
{ | ||||||
projectRoot = ProjectRootElement.Create(xmlReader, projectCollection); | ||||||
} | ||||||
projectRoot.FullPath = projectFileFullPath; | ||||||
return projectRoot; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,14 @@ public static RunCommand FromArgs(string[] args) | |
return FromParseResult(parseResult); | ||
} | ||
|
||
internal static bool IsBinLogArgument(string arg) | ||
{ | ||
const StringComparison comp = StringComparison.Ordinal; | ||
return arg.StartsWith("/bl:", comp) || arg.Equals("/bl", comp) | ||
|| arg.StartsWith("--binaryLogger:", comp) || arg.Equals("--binaryLogger", comp) | ||
|| arg.StartsWith("-bl:", comp) || arg.Equals("-bl", comp); | ||
} | ||
jjonescz marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. || arg.StartsWith("/binaryLogger:") || arg.Equals("/binaryLogger") Casing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For casing, it uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely missed that 👍 |
||
|
||
public static RunCommand FromParseResult(ParseResult parseResult) | ||
{ | ||
if (parseResult.UsingRunCommandShorthandProjectOption()) | ||
|
@@ -35,10 +43,7 @@ public static RunCommand FromParseResult(ParseResult parseResult) | |
var nonBinLogArgs = new List<string>(); | ||
foreach (var arg in applicationArguments) | ||
{ | ||
|
||
if (arg.StartsWith("/bl:") || arg.Equals("/bl") | ||
|| arg.StartsWith("--binaryLogger:") || arg.Equals("--binaryLogger") | ||
|| arg.StartsWith("-bl:") || arg.Equals("-bl")) | ||
if (IsBinLogArgument(arg)) | ||
{ | ||
binlogArgs.Add(arg); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call is now being used in a few places, sometimes to create a new dictionary and sometimes to just iterate this data which is effectively static. Did you consider instead having a definition like this:
Then in the places where a new dictionary is needed use
new (MSBuildRequiredEnvironmentVariables)
and for iteration it can just use the property directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default env vars cannot be static and initialized only once, they can change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still make it static and mutable so that you only have to reinitialize it when it does change, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. Are you suggesting to create a
static
field which contains aDictionary<string, string>
and whenever some of the inputs to this code changewe would change the dictionary accordingly? That sounds like introducing some error-prone logic for what I think would be a negligible benefit.
Also a static mutable field sounds problematic in case of multi-threaded access.