Skip to content
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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,7 @@ public static string MSBuildVersion
// True if, given current state of the class, MSBuild would be executed in its own process.
public bool ExecuteMSBuildOutOfProc => _forwardingApp != null;

private readonly Dictionary<string, string> _msbuildRequiredEnvironmentVariables =
new()
{
{ "MSBuildExtensionsPath", MSBuildExtensionsPathTestHook ?? AppContext.BaseDirectory },
{ "MSBuildSDKsPath", GetMSBuildSDKsPath() },
{ "DOTNET_HOST_PATH", GetDotnetPath() },
};
private readonly Dictionary<string, string> _msbuildRequiredEnvironmentVariables = GetMSBuildRequiredEnvironmentVariables();

private readonly List<string> _msbuildRequiredParameters =
[ "-maxcpucount", "-verbosity:m" ];
Expand Down Expand Up @@ -205,6 +199,16 @@ private static string GetDotnetPath()
return new Muxer().MuxerPath;
}

internal static Dictionary<string, string> GetMSBuildRequiredEnvironmentVariables()
Copy link
Member

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:

 internal static ImmutableDictionary<string, string> MSBuildRequiredEnvironmentVariables { get; } = ...

Then in the places where a new dictionary is needed use new (MSBuildRequiredEnvironmentVariables) and for iteration it can just use the property directly.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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 a Dictionary<string, string> and whenever some of the inputs to this code change

{ "MSBuildExtensionsPath", MSBuildExtensionsPathTestHook ?? AppContext.BaseDirectory },
{ "MSBuildSDKsPath", GetMSBuildSDKsPath() },
{ "DOTNET_HOST_PATH", GetDotnetPath() },

we 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.

{
return new()
{
{ "MSBuildExtensionsPath", MSBuildExtensionsPathTestHook ?? AppContext.BaseDirectory },
{ "MSBuildSDKsPath", GetMSBuildSDKsPath() },
{ "DOTNET_HOST_PATH", GetDotnetPath() },
};
}

private static bool IsRestoreSources(string arg)
{
return arg.StartsWith("/p:RestoreSources=", StringComparison.OrdinalIgnoreCase) ||
Expand Down
194 changes: 194 additions & 0 deletions src/Cli/dotnet/commands/VirtualProjectBuildingCommand.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable enable

using System.Collections.Immutable;
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 binaryLoggers = GetBinaryLoggers(binaryLoggerArgs);
var consoleLogger = new ConsoleLogger(verbosity);
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate? I couldn't find such API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@jjonescz jjonescz Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the SDK intentionally doesn't use terminal logger now, see:

// Temporary fix for 9.0.1xx. 9.0.2xx will use the TerminalLogger in the safe way.
var thing = new ConsoleLogger(msbuildVerbosity);

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())
{
savedEnvironmentVariables[key] = Environment.GetEnvironmentVariable(key);
Environment.SetEnvironmentVariable(key, value);
}

// Setup MSBuild.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit:

Suggested change
// Setup MSBuild.
// Set up MSBuild.

var projectCollection = new ProjectCollection(
GlobalProperties,
[.. binaryLoggers, consoleLogger],
ToolsetDefinitionLocations.Default);
var parameters = new BuildParameters(projectCollection)
{
Loggers = projectCollection.Loggers,
LogTaskInputs = binaryLoggers.Length != 0,
};
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
// and https://github.com/dotnet/msbuild/issues/11519.
var restoreRequest = new BuildRequestData(
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@jjonescz jjonescz Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Performance is presumably better by invoking MSBuild in memory. We are planning to do more optimizations as well (see the design doc), e.g., skipping MSBuild invocation and invoking just csc.exe or even just the target dll. For that, we will need to communicate with MSBuild to determine the csc arguments.
  2. If we create a fake project, it will show up on disk during the build time, which is not a great user experience. We could create it inside obj folder but that's a problem because then it has a different path which is an observable difference.
  3. Hopefully, MSBuild will introduce a public API for implicit restore (see the linked tracking issue) and we can drastically simplify this.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

@jjonescz jjonescz Mar 6, 2025

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@jjonescz jjonescz Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RunCommand does a lot of work that you don't

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see:

  • Launch settings (which we aren't supporting here intentionally)
  • Clean cancelling on ctrl-c
  • Exception handling
  • Restore arguments? (nologo, verbosity; I'm not sure we need this)
  • User-passed properties/targets (I think there's a strong argument that we shouldn't support this)

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) =>
{
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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we creating two separate project instances?

Copy link
Member Author

@jjonescz jjonescz Mar 5, 2025

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BuildRequestData has an overload with global properties

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BuildRequestData has an overload with global properties

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.

Copy link
Member

Choose a reason for hiding this comment

The 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);
}

foreach (var binaryLogger in binaryLoggers)
{
binaryLogger.Shutdown();
}

consoleLogger.Shutdown();
}

static ImmutableArray<ILogger> GetBinaryLoggers(string[] args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't an error for having multiple binlog arguments, but it doesn't actually make a bunch of different binlogs.

image

This should reflect that.

Copy link
Member Author

@jjonescz jjonescz Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks, will fix. It looks like there are some inconsistencies though - dotnet-run binlogs are created for each -bl: argument, see this function:

static FacadeLogger? DetermineBinlogger(string[] restoreArgs)

I will change that as well to make this all consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'm not sure why someone would even want multiple binloggers attached to a single build. I can imagine wanting a separate binlog for different builds that happened as part of some script if they would overwrite each other, but making that FacadeLogger just seems wrong to me.

{
return args
.Where(RunCommand.IsBinLogArgument)
.Select(static ILogger (arg) => new BinaryLogger
{
Parameters = arg.IndexOf(':') is >= 0 and var index
? arg[(index + 1)..]
: "msbuild.binlog",
})
.ToImmutableArray();
}
}

public ProjectInstance CreateProjectInstance(ProjectCollection projectCollection)
{
return CreateProjectInstance(projectCollection, addGlobalProperties: null);
}

private ProjectInstance CreateProjectInstance(
ProjectCollection projectCollection,
Action<IDictionary<string, string>>? addGlobalProperties)
{
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this causes us to use the containing directory of file.cs to gather up .cs files to include in the project? Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. Are you suggesting dotnet run file.cs should only consider file.cs and not the other files in the directory? That's surely a possibility but more of a design question (see also the design doc at #47020). In short, the current design envisions users having multiple "scripts" in one directory like scripts/restore.cs, scripts/build.cs and some shared utilities like scripts/utils.cs which should be accessible from the scripts.

var projectFileText = """
<Project>
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is in main this should be net10.0 - we should have the current TFM available as a constant that you can interpolate into this structure.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@jjonescz jjonescz Mar 5, 2025

Choose a reason for hiding this comment

The 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.

we should have the current TFM available as a constant that you can interpolate into this structure.

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 dotnet new template, so that should ensure we stay consistent.

I got here and planned to make pretty much exactly this comment.

I would actually prefer that, reusing old threads is hard to follow.

<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, dotnet run file.cs is intended be the "default" experience. For anything else, it is recommended to migrate to project-based programs or use directives if they are available.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@jjonescz jjonescz Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you are right, but why we have <nullable>enable</nullable> in project file templates then? Today, they are the starting point for users learning C#. Advanced users could presumably enable that on their own. That's also why dotnet run file.cs just reuses what dotnet new does - there shouldn't be much difference IMO, both are used as "the starting point".

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.

Copy link
Member

Choose a reason for hiding this comment

The 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>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we enable AllowUnsafeBlocks by default?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the dotnet run file.cs feature aims to use the default project template, whatever that is. That makes it predictable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's true, then why did you change the default template?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
See https://github.com/NuGet/Home/issues/14148.
Copy link
Contributor

@nkolev92 nkolev92 Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static graph is not ever going to be a thing with this loose-file usage mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static graph restore never triggers any of the target being reported as a problem, so I thought it'd just skip the problem.
Why do we have a concern with static graph?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to make sure you saw, why is static graph restore a concern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I thought you were asking what if user enabled static graph restore, whether this will work. But you are saying we should enable static graph restore and then perhaps we could avoid this workaround. Thanks, I will investigate that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'd be curious to see the error that's happening and where it's getting thrown from.
    These targets are not invoked at all during static graph restore, so it's surprising if the error is the same.
  2. There's no functional differences between the 2. The biggest reason why static graph is not default is because it's slower for smaller projects because of the extra process.
  3. What are we saying is not supported?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'd be curious to see the error that's happening and where it's getting thrown from.
    These targets are not invoked at all during static graph restore, so it's surprising if the error is the same.

The error is not exactly the same, let me try to figure out where it's coming from.

3. What are we saying is not supported?

Static graph restore in combination with dotnet run file.cs.

Copy link
Contributor

@nkolev92 nkolev92 Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding this scenario, but given that customers don't see a project file, I don't think they care about static graph vs regular restore, they just care that it gets compiled and run.

So I don't think we have a support story for static graph there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but they could do dotnet run file.cs -p:RestoreUseStaticGraphEvaluation=true or more likely have RestoreUseStaticGraphEvaluation set in a parent Directory.Build.props and then if such dotnet run file.cs fails that seems like a bug (unless we say it's unsupported for starters).

Copy link
Member Author

@jjonescz jjonescz Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the error comes from RestoreTaskEx, not sure where exactly but found this test that shows the exact error:

https://github.com/NuGet/NuGet.Client/blob/54f4fd0b4b418666683ab717d075fe0a239eb484/test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs#L821

The error is error MSB4025: The project file could not be loaded. Could not find file '...'.

For repro (which produces a binlog for example), you can use this branch: https://github.com/jjonescz/MSBuildInMemory/tree/static-graph

-->

<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;
}
}
3 changes: 3 additions & 0 deletions src/Cli/dotnet/commands/dotnet-run/LocalizableStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -244,4 +244,7 @@ Make the profile names distinct.</value>
<data name="LaunchProfileDoesNotExist" xml:space="preserve">
<value>A launch profile with the name '{0}' doesn't exist.</value>
</data>
<data name="NoTopLevelStatements" xml:space="preserve">
<value>Cannot run a file without top-level statements and without a project: '{0}'</value>
</data>
</root>
13 changes: 9 additions & 4 deletions src/Cli/dotnet/commands/dotnet-run/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ public static RunCommand FromArgs(string[] args)
return FromParseResult(parseResult);
}

internal static bool IsBinLogArgument(string arg)
{
const StringComparison comp = StringComparison.OrdinalIgnoreCase;
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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| arg.StartsWith("/binaryLogger:") || arg.Equals("/binaryLogger")

Casing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For casing, it uses StringComparison.OrdinalIgnoreCase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely missed that 👍


public static RunCommand FromParseResult(ParseResult parseResult)
{
if (parseResult.UsingRunCommandShorthandProjectOption())
Expand All @@ -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);
}
Expand Down
Loading