Skip to content

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Sep 5, 2025

Part of #80151.

@jjonescz jjonescz marked this pull request as ready for review September 5, 2025 19:41
@jjonescz jjonescz requested review from a team as code owners September 5, 2025 19:41
return RuntimeHostInfo.GetDotNetPathOrDefault();
}

if (UsingBuiltinTool)
Copy link
Member

Choose a reason for hiding this comment

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

We already know UsingBuiltinTool is true from the outer check.

Comment on lines 166 to 171
if (IsBuiltinToolRunningOnCoreClr && !UseAppHost)
{
return $"{ToolNameWithoutExtension}.dll";
}

return AppHostToolName;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (IsBuiltinToolRunningOnCoreClr && !UseAppHost)
{
return $"{ToolNameWithoutExtension}.dll";
}
return AppHostToolName;
Debug.Assert(IsBuiltinToolRunningOnCoreClr || UseAppHost);
return UseAppHost ? AppHostToolName : $"{ToolNameWithoutExtension}.dll";

return GenerateToolArguments();
var commandLineArguments = GenerateToolArguments();

if (UsingBuiltinTool && IsBuiltinToolRunningOnCoreClr && !UseAppHost)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (UsingBuiltinTool && IsBuiltinToolRunningOnCoreClr && !UseAppHost)
if (UsingBuiltinTool && !UseAppHost)

Can't the condition be simplified to this? Cause whenever !UseAppHost is true we must be IsBuiltinToolRunningOnCoreClr (I think at least)

var task = new Csc();
Assert.Equal(Path.Combine(taskPath, "..", "bincore", $"csc{PlatformInformation.ExeExtension}"), task.PathToBuiltInTool);
Assert.Contains(task.PathToBuiltInTool,
new[] {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
new[] {
new[]
{

@RikkiGibson
Copy link
Member

I see other folks are engaged here so removing my assignment. Feel free to re-assign if I am needed here.

@RikkiGibson RikkiGibson removed their request for review September 9, 2025 18:54
@jjonescz jjonescz merged commit 984aa24 into dotnet:main Sep 11, 2025
28 checks passed
@jjonescz jjonescz deleted the apphost-toolset branch September 11, 2025 07:04
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Sep 11, 2025
333fred added a commit to 333fred/roslyn that referenced this pull request Sep 11, 2025
* upstream/main: (233 commits)
  Extensions: add SyntaxGenerator support and AssociatedExtensionImplementation API (dotnet#80170)
  Fix error when hoisting a non-ref call (dotnet#80138)
  Ensure that refkinds are rewritten for complex methods (dotnet#79916)
  Revert
  Do not go through the workspace to access services
  DefiniteAssignmentPass.MarkFieldsUsed - avoid infinite recursion due to generic substitution (dotnet#80135)
  Reduce allocations in AnalyzerDriver.TryExecuteSymbolEndActions (dotnet#79855)
  RefSafetyAnalysis: Fix handling of nested deconstruction utilizing modern extensions (dotnet#80231)
  Extensions: adjust rewriting of anonymous type property symbols (dotnet#80211)
  Extensions: Move public APIs into INamedTypeSymbol (dotnet#80230)
  Extensions: improve error recovery in older language versions (dotnet#80206)
  Fall back to `dotnet exec` if apphost does not exist (dotnet#80153)
  Update dependencies from https://github.com/dotnet/dotnet build 282708 (dotnet#80228)
  Add a workaround for microsoft/vs-mef#620
  Revert "FailFast if the MEF composition is clearly broken"
  switch from windows combobox to visualstudio combobox (dotnet#80219)
  Update System.Text.Json in packages which use 4.12 Roslyn (dotnet#80197)
  add flags to unblock CI (dotnet#80222)
  Move static members to another type - qualifies static member references in the moved members (dotnet#80178)
  Fix broken link for C# 14 lambda parameter modifiers
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants