Add basic support for 'dotnet run file.cs' #46915

jjonescz · 2025-02-18T14:21:30Z

Copilot

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/Cli/dotnet/commands/dotnet-run/RunCommand.cs:609

[nitpick] The method 'DiscoverProjectFilePath' now returns both a project file path and an entry point file via an out parameter; consider renaming it (for example, to 'DiscoverProjectAndEntryPointPaths') to better reflect its dual purpose.

private static string? DiscoverProjectFilePath(string? projectFileOrDirectoryPath, ref string[] args, out string? entryPointFilePath)

src/Cli/dotnet/commands/dotnet-run/RunCommand.cs:250

[nitpick] Consider renaming the 'projectFactory' out parameter to 'projectInstanceFactory' for improved clarity since it is intended to supply a ProjectInstance.

private void EnsureProjectIsBuilt(out Func<ProjectCollection, ProjectInstance>? projectFactory)

Copilot · 2025-02-19T12:16:51Z

src/Cli/dotnet/commands/dotnet-run/RunCommand.cs

+                    {
+                        if (globalProperties.TryGetValue(key, out var existingValues))
+                        {
+                            globalProperties[key] = existingValues + ";" + value;


[nitpick] Concatenating multiple property values with a semicolon might lead to formatting issues if the existing value already ends with one; consider aggregating values in a list and joining them to ensure proper formatting.

Suggested change

globalProperties[key] = existingValues + ";" + value;

var valuesList = existingValues.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries).ToList();

valuesList.Add(value);

globalProperties[key] = string.Join(";", valuesList);

If user is passing empty properties somehow, I think that's on them. Also, this should be equivalent to pre-existing code, just moved.

jaredpar · 2025-02-19T22:34:17Z

src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs

@@ -205,6 +199,16 @@ private static string GetDotnetPath()
            return new Muxer().MuxerPath;
        }

+        internal static Dictionary<string, string> GetMSBuildRequiredEnvironmentVariables()
+        {
+            return new()


I know this is code that is already there but surprised this is using an ordinal comparison for environment variable names.

Good catch, I guess I'm keeping this since it's a pre-existing behavior, but let me know if you think I should change this.

jaredpar · 2025-02-19T22:35:28Z

src/Cli/dotnet/commands/VirtualProjectBuildingCommand.cs

+            for (int i = args.Length - 1; i >= 0; i--)
+            {
+                var arg = args[i];
+                if (arg.StartsWith("/bl:") || arg.Equals("/bl")


Use explicit string comparison arguments

Good idea. This code was copied from elsewhere, let me share it.

It looks like dotnet build is case insensitive (dotnet build -BL produces a binlog) whereas dotnet run is case sensitive (-BL is passed as an argument to the executed app), so I'm not sure if I should fix that inconsistency and use ignore-case comparison here to make dotnet run -BL produce binlog as well (but that's technically a breaking change)?

jaredpar · 2025-02-19T22:39:27Z

src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs

@@ -205,6 +199,16 @@ private static string GetDotnetPath()
            return new Muxer().MuxerPath;
        }

+        internal static Dictionary<string, string> GetMSBuildRequiredEnvironmentVariables()


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.

The default env vars cannot be static and initialized only once, they can change.

jaredpar · 2025-02-19T22:43:38Z

src/Cli/dotnet/commands/VirtualProjectBuildingCommand.cs

+        }
+    }
+
+    public ProjectInstance CreateProjectInstance(ProjectCollection projectCollection)


Is there a reason this overload is needed? The below one with the optional should work for the call sites I see.

Yes, there is a delegate created from this one here:

sdk/src/Cli/dotnet/commands/dotnet-run/RunCommand.cs

Line 262 in 8094d49

projectFactory = command.CreateProjectInstance;

The one with the optional argument wouldn't work (would need a lambda wrapper which seems less efficient).

Also this one is public whereas the one with the optional arg isn't exposed, it's private.

jaredpar · 2025-02-19T22:45:00Z

src/Cli/dotnet/commands/VirtualProjectBuildingCommand.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#nullable enable


Is this needed? The other files seem to implicitly have nullable analysis enabled

They unfortunately don't, only nullable annotations are enabled, but you get no nullable warnings without this.

Which made me realize I could also nullable-enable the RunCommand.cs file since I did a lot of changes in there too.

Add basic support for 'dotnet run file.cs'

aea7240

dotnet-issue-labeler bot added Area-CLI untriaged Request triage from a team member labels Feb 18, 2025

Fix tests on case sensitive OSes

8094d49

jjonescz force-pushed the sprint branch from e62e7f5 to 8094d49 Compare February 18, 2025 17:24

jjonescz requested review from DamianEdwards, baronfel and jaredpar February 19, 2025 12:15

jjonescz marked this pull request as ready for review February 19, 2025 12:15

Copilot bot review requested due to automatic review settings February 19, 2025 12:15

Copilot AI reviewed Feb 19, 2025

View reviewed changes

jaredpar requested review from chsienki and RikkiGibson February 19, 2025 22:27

jaredpar reviewed Feb 19, 2025

View reviewed changes

RikkiGibson self-assigned this Feb 19, 2025

jjonescz added 2 commits February 20, 2025 09:53

Share binlog arg detection code

9f51756

Nullable-enable RunCommand.cs

5a5206f

jjonescz force-pushed the sprint branch from d36a366 to 5a5206f Compare February 20, 2025 11:53

This was referenced Feb 20, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

dotnet-watch tests failing #40006

Open

[dotnet watch] Cannot open project Lib.fsproj because extension fsproj is not associated with a language #46378

Open

jaredpar requested a review from MiYanni February 20, 2025 19:13

Test embedded resource

254eccb

jjonescz mentioned this pull request Feb 21, 2025

Add spec for dotnet run file.cs #47020

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support for 'dotnet run file.cs' #46915

Add basic support for 'dotnet run file.cs' #46915

jjonescz commented Feb 18, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI Feb 19, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

jjonescz Feb 20, 2025

jaredpar Feb 19, 2025

jjonescz Feb 20, 2025

jaredpar Feb 19, 2025

jjonescz Feb 20, 2025 •

edited

Loading

jaredpar Feb 19, 2025

jjonescz Feb 20, 2025

jaredpar Feb 19, 2025

jjonescz Feb 20, 2025

jaredpar Feb 19, 2025

jjonescz Feb 20, 2025 •

edited

Loading

-                            globalProperties[key] = existingValues + ";" + value;
+                            var valuesList = existingValues.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries).ToList();
+                            valuesList.Add(value);
+                            globalProperties[key] = string.Join(";", valuesList);

Add basic support for 'dotnet run file.cs' #46915

Are you sure you want to change the base?

Add basic support for 'dotnet run file.cs' #46915

Conversation

jjonescz commented Feb 18, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Feb 19, 2025

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

jjonescz Feb 20, 2025 • 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

jjonescz Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

jjonescz commented Feb 18, 2025 •

edited

Loading

jjonescz Feb 20, 2025 •

edited

Loading

jjonescz Feb 20, 2025 •

edited

Loading