Skip to content

Commit

Permalink
Remove host specific args from CommandLineArgs (#68918)
Browse files Browse the repository at this point in the history 
This changes our build tasks to remove the host specific args, namely the `exec ...\csc.dll`, from the `CommandLineArgs` property. That `ITaskItem[]` is meant for the IDE consumption and doesn't need host specific information. 

As a part of fixing this bug I cleaned up a lot of unnecessary member inheritance in the build tasks. There were several `virtual` members where every implementation had the same code. Moved all that up to the base and cleaned up the comments to make the intent clearer. 

closes #67102

Co-authored-by: Jared Parsons <jared@popcornbear.org>
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
  • Loading branch information
@jaredpar @chsienki
3 people authored Jul 11, 2023
1 parent 5731c5f commit 854fa07
Show file tree
Hide file tree
Showing 13 changed files with 322 additions and 207 deletions.
2 changes: 1 addition & 1 deletion src/Compilers/Core/MSBuildTask/Csc.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ protected override string ToolNameWithoutExtension
/// <summary>
/// Fills the provided CommandLineBuilderExtension with those switches and other information that can go into a response file.
/// </summary>
protected internal override void AddResponseFileCommands(CommandLineBuilderExtension commandLine)
protected override void AddResponseFileCommands(CommandLineBuilderExtension commandLine)
{
commandLine.AppendSwitchIfNotNull("/lib:", AdditionalLibPaths, ",");
commandLine.AppendPlusOrMinusSwitch("/unsafe", _store, nameof(AllowUnsafeBlocks));
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/Core/MSBuildTask/Csi.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ public class Csi : InteractiveCompiler
#endregion

#region Interactive Compiler Members

protected override void AddCommandLineCommands(CommandLineBuilderExtension commandLine)
{
// Nothing to add
}

protected override void AddResponseFileCommands(CommandLineBuilderExtension commandLine)
{
commandLine.AppendSwitchIfNotNull("/lib:", AdditionalLibPaths, ",");
Expand Down
52 changes: 2 additions & 50 deletions src/Compilers/Core/MSBuildTask/InteractiveCompiler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,60 +184,22 @@ public ITaskItem? Source

#region Tool Members

// See ManagedCompiler.cs on the logic of this property
private bool HasToolBeenOverridden => !(string.IsNullOrEmpty(ToolPath) && ToolExe == ToolName);

protected sealed override bool IsManagedTool => !HasToolBeenOverridden;

protected sealed override string PathToManagedTool => Utilities.GenerateFullPathToTool(ToolName);

protected sealed override string PathToNativeTool => Path.Combine(ToolPath ?? "", ToolExe);

protected override int ExecuteTool(string pathToTool, string responseFileCommands, string commandLineCommands)
{
if (ProvideCommandLineArgs)
{
CommandLineArgs = GetArguments(commandLineCommands, responseFileCommands).Select(arg => new TaskItem(arg)).ToArray();
CommandLineArgs = GenerateCommandLineArgsTaskItems(responseFileCommands);
}

return (SkipInteractiveExecution) ? 0 : base.ExecuteTool(pathToTool, responseFileCommands, commandLineCommands);
}

public string GenerateCommandLineContents() => GenerateCommandLineCommands();

protected sealed override string ToolArguments
{
get
{
var builder = new CommandLineBuilderExtension();
AddCommandLineCommands(builder);
return builder.ToString();
}
}

public string GenerateResponseFileContents() => GenerateResponseFileCommands();

protected sealed override string GenerateResponseFileCommands()
{
var commandLineBuilder = new CommandLineBuilderExtension();
AddResponseFileCommands(commandLineBuilder);
return commandLineBuilder.ToString();
}

#endregion

/// <summary>
/// Fills the provided CommandLineBuilderExtension with those switches and other information that can't go into a response file and
/// must go directly onto the command line.
/// </summary>
protected virtual void AddCommandLineCommands(CommandLineBuilderExtension commandLine)
{
}

/// <summary>
/// Fills the provided CommandLineBuilderExtension with those switches and other information that can go into a response file.
/// </summary>
protected virtual void AddResponseFileCommands(CommandLineBuilderExtension commandLine)
protected override void AddResponseFileCommands(CommandLineBuilderExtension commandLine)
{
commandLine.AppendSwitch("/i-");

Expand Down Expand Up @@ -269,15 +231,5 @@ protected virtual void AddResponseFileCommands(CommandLineBuilderExtension comma
}
}
}

/// <summary>
/// Get the command line arguments to pass to the compiler.
/// </summary>
private string[] GetArguments(string commandLineCommands, string responseFileCommands)
{
var commandLineArguments = CommandLineUtilities.SplitCommandLineIntoArguments(commandLineCommands, removeHashComments: true);
var responseFileArguments = CommandLineUtilities.SplitCommandLineIntoArguments(responseFileCommands, removeHashComments: true);
return commandLineArguments.Concat(responseFileArguments).ToArray();
}
}
}
71 changes: 5 additions & 66 deletions src/Compilers/Core/MSBuildTask/ManagedCompiler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,15 +477,6 @@ public bool ReportIVTs

#endregion

// ToolExe delegates back to ToolName if the override is not
// set. So, if ToolExe == ToolName, we know ToolExe is not
// explicitly overridden. So, if both ToolPath is unset and
// ToolExe == ToolName, we know nothing is overridden, and
// we can use our own csc.
private bool HasToolBeenOverridden => !(string.IsNullOrEmpty(ToolPath) && ToolExe == ToolName);

protected sealed override bool IsManagedTool => !HasToolBeenOverridden;

/// <summary>
/// Method for testing only
/// </summary>
Expand All @@ -494,10 +485,6 @@ public string GeneratePathToTool()
return GenerateFullPathToTool();
}

protected sealed override string PathToManagedTool => Utilities.GenerateFullPathToTool(ToolName);

protected sealed override string PathToNativeTool => Path.Combine(ToolPath ?? "", ToolExe);

protected override int ExecuteTool(string pathToTool, string responseFileCommands, string commandLineCommands)
{
using var logger = new CompilerServerLogger($"MSBuild {Process.GetCurrentProcess().Id}");
Expand All @@ -508,8 +495,7 @@ internal int ExecuteTool(string pathToTool, string responseFileCommands, string
{
if (ProvideCommandLineArgs)
{
CommandLineArgs = GetArguments(commandLineCommands, responseFileCommands)
.Select(arg => new TaskItem(arg)).ToArray();
CommandLineArgs = GenerateCommandLineArgsTaskItems(responseFileCommands);
}

if (SkipCompilerExecution)
Expand All @@ -526,7 +512,7 @@ internal int ExecuteTool(string pathToTool, string responseFileCommands, string
string? tempDirectory = BuildServerConnection.GetTempPath(workingDirectory);

if (!UseSharedCompilation ||
HasToolBeenOverridden ||
!IsManagedTool ||
!BuildServerConnection.IsCompilerServerSupported)
{
LogCompilationMessage(logger, requestId, CompilationKind.Tool, $"using command line tool by design '{pathToTool}'");
Expand All @@ -550,7 +536,7 @@ internal int ExecuteTool(string pathToTool, string responseFileCommands, string
var buildRequest = BuildServerConnection.CreateBuildRequest(
requestId,
Language,
GetArguments(ToolArguments, responseFileCommands).ToList(),
GenerateCommandLineArgsList(responseFileCommands),
workingDirectory: workingDirectory,
tempDirectory: tempDirectory,
keepAlive: null,
Expand Down Expand Up @@ -804,66 +790,19 @@ private void LogCompilationMessage(ICompilerServerLogger logger, Guid requestId,
}
}

public string GenerateResponseFileContents()
{
return GenerateResponseFileCommands();
}

/// <summary>
/// Get the command line arguments to pass to the compiler.
/// </summary>
private string[] GetArguments(string commandLineCommands, string responseFileCommands)
{
var commandLineArguments =
CommandLineUtilities.SplitCommandLineIntoArguments(commandLineCommands, removeHashComments: true);
var responseFileArguments =
CommandLineUtilities.SplitCommandLineIntoArguments(responseFileCommands, removeHashComments: true);
return commandLineArguments.Concat(responseFileArguments).ToArray();
}

/// <summary>
/// Returns the command line switch used by the tool executable to specify the response file
/// Will only be called if the task returned a non empty string from GetResponseFileCommands
/// Called after ValidateParameters, SkipTaskExecution and GetResponseFileCommands
/// </summary>
protected override string GenerateResponseFileCommands()
{
CommandLineBuilderExtension commandLineBuilder = new CommandLineBuilderExtension();
AddResponseFileCommands(commandLineBuilder);
return commandLineBuilder.ToString();
}

/// <summary>
/// Method for testing only
/// </summary>
public string GenerateCommandLine()
{
return GenerateCommandLineCommands();
}

protected sealed override string ToolArguments
{
get
{
var builder = new CommandLineBuilderExtension();
AddCommandLineCommands(builder);
return builder.ToString();
}
}

/// <summary>
/// Fills the provided CommandLineBuilderExtension with those switches and other information that can't go into a response file and
/// must go directly onto the command line.
/// </summary>
protected internal virtual void AddCommandLineCommands(CommandLineBuilderExtension commandLine)
protected override void AddCommandLineCommands(CommandLineBuilderExtension commandLine)
{
commandLine.AppendWhenTrue("/noconfig", _store, nameof(NoConfig));
}

/// <summary>
/// Fills the provided CommandLineBuilderExtension with those switches and other information that can go into a response file.
/// </summary>
protected internal virtual void AddResponseFileCommands(CommandLineBuilderExtension commandLine)
protected override void AddResponseFileCommands(CommandLineBuilderExtension commandLine)
{
// If outputAssembly is not specified, then an "/out: <name>" option won't be added to
// overwrite the one resulting from the OutputAssembly member of the CompilerParameters class.
Expand Down
Loading

0 comments on commit 854fa07

Please sign in to comment.