Include default RSP files for core compilers #80993
+245
−20
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,5 +22,10 @@ | |
| <InternalsVisibleTo Include="Microsoft.Build.Tasks.CodeAnalysis.Sdk.UnitTests" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <None Include="$(MSBuildThisFileDirectory)..\..\..\CSharp\csc\csc.rsp" CopyToOutputDirectory="PreserveNewest" /> | ||
|
Contributor
Member
Author
There was a problem hiding this comment. This is inspired by other projects that include csc.rsp: It works by not copying the file if it's not modified, so the builds are more performant
Contributor
There was a problem hiding this comment.
That is great, but I still would like to understand how exactly this option works. Other projects could have made a mistake. Since these files are not build artifacts, it doesn't feel appropriate to me to copy files only when they are newer. BTW, what exactly does it mean for a file to be newer for the purpose of this operation?
Member
Author
There was a problem hiding this comment. The file is copied to the output directory only if the source file has newer timestamp than the target file (the one in the output directory of the project). I think this is the common default for many MSBuild operations. It's documented here: https://learn.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-items?view=vs-2022#none Looks like we could use IfDifferent (but that's a newer option: dotnet/docs#44059); or Always. |
||
| <None Include="$(MSBuildThisFileDirectory)..\..\..\VisualBasic\vbc\vbc.rsp" CopyToOutputDirectory="PreserveNewest" /> | ||
| </ItemGroup> | ||
|
|
||
| <Import Project="..\..\..\..\Dependencies\Contracts\Microsoft.CodeAnalysis.Contracts.projitems" Label="Shared" /> | ||
| </Project> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,8 +187,8 @@ End Module | |
| Assert.Contains(ExecutionConditionUtil.IsWindows ? "vbc.exe" : "vbc", result.Output); | ||
| } | ||
|
|
||
| [Theory, PairwiseData, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
| public void StdLib_Csc(bool useSharedCompilation, bool disableSdkPath, bool noConfig) | ||
| { | ||
| if (_msbuildExecutable == null) return; | ||
|
|
||
|
|
@@ -199,35 +199,49 @@ public void StdLib_Csc(bool useSharedCompilation, bool disableSdkPath) | |
| new Dictionary<string, string> | ||
| { | ||
| { "File.cs", """ | ||
| using System.Linq; | ||
| System.Console.WriteLine("Hello from file"); | ||
| """ }, | ||
| { "Test.csproj", $""" | ||
| <Project> | ||
| <UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Csc" AssemblyFile="{_buildTaskDll}" /> | ||
| <Target Name="CustomTarget"> | ||
| <Csc Sources="File.cs" UseSharedCompilation="{useSharedCompilation}" DisableSdkPath="{disableSdkPath}" NoConfig="{noConfig}" /> | ||
|
Contributor
Member
Author
There was a problem hiding this comment. It avoids exponential amount of test runs since I added new input. See https://aarnott.github.io/Xunit.Combinatorial/docs/combinatorial-vs-pairwise.html. The pairwise ones should be the interesting ones (I've verified the test still fails without the product change for example).
Contributor
There was a problem hiding this comment. I do not find 8 cases too much and would be more comfortable with covering full matrix. I am uncomfortable with a situation when an "external" tool decides what combinations our tests cover and that combinations possibly changing between versions of that tool. My recommendation is to use full matrix or explicitly specifying combinations covered by the test. |
||
| </Target> | ||
| </Project> | ||
| """ }, | ||
| }); | ||
| _output.WriteLine(result.Output); | ||
|
|
||
| if (disableSdkPath || noConfig) | ||
| { | ||
| Assert.NotEqual(0, result.ExitCode); | ||
| if (disableSdkPath && noConfig) | ||
| { | ||
| // error CS0246: The type or namespace name 'System' could not be found | ||
| Assert.Contains("error CS0246", result.Output); | ||
| } | ||
| else if (disableSdkPath) | ||
| { | ||
| // error CS0006: Metadata file could not be found | ||
| Assert.Contains("error CS0006", result.Output); | ||
| } | ||
| else | ||
| { | ||
| // error CS0234: The type or namespace name 'Linq' does not exist in the namespace 'System' | ||
| Assert.Contains("error CS0234", result.Output); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Assert.Equal(0, result.ExitCode); | ||
| } | ||
|
|
||
| Assert.Contains(useSharedCompilation ? "server processed compilation" : "using command line tool by design", result.Output); | ||
| } | ||
|
|
||
| [Theory, PairwiseData, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
| public void StdLib_Vbc(bool useSharedCompilation, bool disableSdkPath, bool noConfig) | ||
| { | ||
| if (_msbuildExecutable == null) return; | ||
|
|
||
|
|
@@ -240,31 +254,136 @@ public void StdLib_Vbc(bool useSharedCompilation, bool disableSdkPath) | |
| { "File.vb", """ | ||
| Public Module Program | ||
| Public Sub Main() | ||
| Console.WriteLine("Hello from file") | ||
| End Sub | ||
| End Module | ||
| """ }, | ||
| { "Test.vbproj", $""" | ||
| <Project> | ||
| <UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Vbc" AssemblyFile="{_buildTaskDll}" /> | ||
| <Target Name="CustomTarget"> | ||
| <Vbc Sources="File.vb" UseSharedCompilation="{useSharedCompilation}" DisableSdkPath="{disableSdkPath}" NoConfig="{noConfig}" /> | ||
| </Target> | ||
| </Project> | ||
| """ }, | ||
| }); | ||
| _output.WriteLine(result.Output); | ||
|
|
||
| if (disableSdkPath || noConfig) | ||
| { | ||
| Assert.NotEqual(0, result.ExitCode); | ||
| if (disableSdkPath) | ||
| { | ||
| // error BC2017: could not find library 'Microsoft.VisualBasic.dll' | ||
| Assert.Contains("error BC2017", result.Output); | ||
| } | ||
| else | ||
| { | ||
| Assert.True(noConfig); | ||
| // error BC30451: 'Console' is not declared. It may be inaccessible due to its protection level. | ||
| Assert.Contains("error BC30451", result.Output); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Assert.Equal(0, result.ExitCode); | ||
| } | ||
|
|
||
| Assert.Contains(useSharedCompilation ? "server processed compilation" : "using command line tool by design", result.Output); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Verifies that both RSPs are included: the default <c>csc.rsp</c> (which has <c>/r:System.Data.OracleClient</c>), | ||
| /// and the custom RSP (which has <c>/warnaserror+</c> so we get an error for using an obsolete type). | ||
| /// </summary> | ||
| [Theory, PairwiseData] | ||
| public void CustomRsp_Csc(bool includeCustomRsp, bool useSharedCompilation, bool noConfig) | ||
| { | ||
| if (_msbuildExecutable == null) return; | ||
|
|
||
| var result = RunCommandLineCompiler( | ||
| _msbuildExecutable, | ||
| "/m /nr:false /t:CustomTarget Test.csproj", | ||
| _tempDirectory, | ||
| new Dictionary<string, string> | ||
| { | ||
| { "File.cs", """ | ||
| new System.Data.OracleClient.OracleConnection(""); | ||
| """ }, | ||
| { "custom.rsp", """ | ||
| /warnaserror+ | ||
| """ }, | ||
| { "Test.csproj", $""" | ||
| <Project> | ||
| <UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Csc" AssemblyFile="{_buildTaskDll}" /> | ||
| <Target Name="CustomTarget"> | ||
| <Csc Sources="File.cs" UseSharedCompilation="{useSharedCompilation}" ResponseFiles="{(includeCustomRsp ? "custom.rsp" : "")}" NoConfig="{noConfig}" /> | ||
| </Target> | ||
| </Project> | ||
| """ }, | ||
| }); | ||
| _output.WriteLine(result.Output); | ||
|
|
||
| Assert.Equal(!includeCustomRsp && !noConfig, 0 == result.ExitCode); | ||
| if (noConfig) | ||
| { | ||
| // error CS0234: The type or namespace name 'Data' does not exist in the namespace 'System' | ||
| Assert.Contains("error CS0234", result.Output); | ||
| } | ||
| else | ||
| { | ||
| // warning CS0618: The type is obsolete | ||
| Assert.Contains($"{(includeCustomRsp ? "error" : "warning")} CS0618", result.Output); | ||
| } | ||
|
|
||
| Assert.Contains(useSharedCompilation ? "server processed compilation" : "using command line tool by design", result.Output); | ||
| } | ||
|
|
||
| /// <inheritdoc cref="CustomRsp_Csc"/> | ||
| [Theory, PairwiseData] | ||
| public void CustomRsp_Vbc(bool includeCustomRsp, bool useSharedCompilation, bool noConfig) | ||
| { | ||
| if (_msbuildExecutable == null) return; | ||
|
|
||
| var result = RunCommandLineCompiler( | ||
| _msbuildExecutable, | ||
| "/m /nr:false /t:CustomTarget Test.vbproj", | ||
| _tempDirectory, | ||
| new Dictionary<string, string> | ||
| { | ||
| { "File.vb", """ | ||
| Public Module Program | ||
| Public Sub Main() | ||
| Dim x As Object = New System.Data.OracleClient.OracleConnection("") | ||
| End Sub | ||
| End Module | ||
| """ }, | ||
| { "custom.rsp", """ | ||
| /warnaserror+ | ||
| """ }, | ||
| { "Test.vbproj", $""" | ||
| <Project> | ||
| <UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Vbc" AssemblyFile="{_buildTaskDll}" /> | ||
| <Target Name="CustomTarget"> | ||
| <Vbc Sources="File.vb" UseSharedCompilation="{useSharedCompilation}" ResponseFiles="{(includeCustomRsp ? "custom.rsp" : "")}" NoConfig="{noConfig}" /> | ||
| </Target> | ||
| </Project> | ||
| """ }, | ||
| }); | ||
| _output.WriteLine(result.Output); | ||
|
|
||
| Assert.Equal(!includeCustomRsp && !noConfig, 0 == result.ExitCode); | ||
| if (noConfig) | ||
| { | ||
| // error BC30002: Type 'System.Data.OracleClient.OracleConnection' is not defined. | ||
| Assert.Contains("error BC30002", result.Output); | ||
| } | ||
| else | ||
| { | ||
| // warning BC40000: The type is obsolete | ||
| Assert.Contains($"{(includeCustomRsp ? "error" : "warning")} BC40000", result.Output); | ||
| } | ||
|
|
||
| Assert.Contains(useSharedCompilation ? "server processed compilation" : "using command line tool by design", result.Output); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
How is the defaut RSP file specified in the framework based build?
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.
It is deployed alongside
csc.exe. The compiler then checks the file exists and includes it automatically:roslyn/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs
Lines 130 to 133 in a065eae
I first tried to do that for the core compiler too but that would be a breaking change (could cause errors if the DLLs specified in the response file do not exist).
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.
Overall I get what this PR is trying to accomplish and why we need to do it. What I'm trying to square in my mind is how we limit it to the cases that need it and think through the cases going forward.
Short term you're attaching this logic to coming from the bridge task which makes sense. And really the only way I can see us getting this in now is doing exactly this. In the long term though I'm not sure that is the right approach. Love or hate the raw
<Csc>in targets files customers do it. As customers migrate frommsbuildtodotnet buildI'm guessing the need for raw<Csc>in targets will remain. Hence over the long term I think we will need to support this when running underdotnet build. That is harder because there is no way, today at least, to find the sdk reference path (see teams chat). That means we can't do a no args compile indotnet build.Basically I think in the long term we'll need support for the
dotnet buildinfra to provide path to the appropriateMicrosoft.NetCore.App.Refpackage. That would then translate to/sdkpathand we could have a simple default.rspfor the core compiler. Then<Csc>would work there. This is a change across a few components so we probably should start tracking that.Now that I typed a lot ... back to what to do about this PR
I'm thinking that we should probably only do this when:
/noconfigNoConfigis not passed (yes a double negative)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.
@rainersigwald, @baronfel this is why I've been aksing about finding the sdk path inside of
dotnet buildThere 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 by this. The Csc task doesn't see the current TargetFramework (and adding a new parameter for it could introduce more breaks as we seen recently). And the SDK passes
NoConfig=truewhen TF is specified, so it sounds like (3) supersedes (2)?Regarding (1) and (3), those are what this PR indeed checks. Specifically, only the bridge task will include the default csc.rsp if it exists and
NoConfig==false.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.
It's one of the existing arguments. See here
Where is that done? If that is already the case then I'd feel better.
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.
Ah interesting, thanks!
Well, the SDK passes NoConfig=true always here.