-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Include default RSP files for core compilers #80993
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
cb1c532
92d3e66
a3c20e6
9db4d4f
35e173a
4ae086d
e31f948
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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> | ||||
| 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, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
| public void StdLib_Csc(bool useSharedCompilation, bool disableSdkPath) | ||
| [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,20 +199,21 @@ 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}" /> | ||
| <Csc Sources="File.cs" UseSharedCompilation="{useSharedCompilation}" DisableSdkPath="{disableSdkPath}" NoConfig="{noConfig}" /> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) | ||
| if (disableSdkPath || noConfig) | ||
| { | ||
| Assert.NotEqual(0, result.ExitCode); | ||
| // Either error CS0006: Metadata file could not be found | ||
|
|
@@ -226,8 +227,8 @@ public void StdLib_Csc(bool useSharedCompilation, bool disableSdkPath) | |
| } | ||
| } | ||
|
|
||
| [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
| public void StdLib_Vbc(bool useSharedCompilation, bool disableSdkPath) | ||
| [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,26 +241,35 @@ public void StdLib_Vbc(bool useSharedCompilation, bool disableSdkPath) | |
| { "File.vb", """ | ||
| Public Module Program | ||
| Public Sub Main() | ||
| System.Console.WriteLine("Hello from file") | ||
| 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}" /> | ||
| <Vbc Sources="File.vb" UseSharedCompilation="{useSharedCompilation}" DisableSdkPath="{disableSdkPath}" NoConfig="{noConfig}" /> | ||
| </Target> | ||
| </Project> | ||
| """ }, | ||
| }); | ||
| _output.WriteLine(result.Output); | ||
|
|
||
| if (disableSdkPath) | ||
| if (disableSdkPath || noConfig) | ||
| { | ||
| Assert.NotEqual(0, result.ExitCode); | ||
| // error BC2017: could not find library 'Microsoft.VisualBasic.dll' | ||
| Assert.Contains("error BC2017", result.Output); | ||
| 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 | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.IO; | ||
| using System.Runtime.InteropServices; | ||
| using Microsoft.CodeAnalysis.BuildTasks.UnitTests; | ||
| using Roslyn.Test.Utilities; | ||
|
|
@@ -11,6 +12,8 @@ namespace Microsoft.CodeAnalysis.BuildTasks.Sdk.UnitTests; | |
|
|
||
| public sealed class CscTests | ||
| { | ||
| private static string RspFilePath => Path.Combine(Path.GetDirectoryName(typeof(ManagedCompiler).Assembly.Location)!, "csc.rsp"); | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
| public void StdLib() | ||
| { | ||
|
|
@@ -19,7 +22,7 @@ public void StdLib() | |
| Sources = MSBuildUtil.CreateTaskItems("test.cs"), | ||
| }; | ||
|
|
||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} /out:test.exe test.cs", csc.GenerateResponseFileContents()); | ||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} @{RspFilePath} /out:test.exe test.cs", csc.GenerateResponseFileContents()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
|
|
@@ -31,6 +34,6 @@ public void StdLib_DisableSdkPath() | |
| DisableSdkPath = true, | ||
| }; | ||
|
|
||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} /nosdkpath /out:test.exe test.cs", csc.GenerateResponseFileContents()); | ||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} @{RspFilePath} /nosdkpath /out:test.exe test.cs", csc.GenerateResponseFileContents()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.IO; | ||
| using System.Runtime.InteropServices; | ||
| using Microsoft.CodeAnalysis.BuildTasks.UnitTests; | ||
| using Roslyn.Test.Utilities; | ||
|
|
@@ -11,6 +12,8 @@ namespace Microsoft.CodeAnalysis.BuildTasks.Sdk.UnitTests; | |
|
|
||
| public sealed class VbcTests | ||
| { | ||
| private static string RspFilePath => Path.Combine(Path.GetDirectoryName(typeof(ManagedCompiler).Assembly.Location)!, "vbc.rsp"); | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
| public void StdLib() | ||
| { | ||
|
|
@@ -19,7 +22,7 @@ public void StdLib() | |
| Sources = MSBuildUtil.CreateTaskItems("test.vb"), | ||
| }; | ||
|
|
||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} /optionstrict:custom /out:test.exe test.vb", vbc.GenerateResponseFileContents()); | ||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} @{RspFilePath} /optionstrict:custom /out:test.exe test.vb", vbc.GenerateResponseFileContents()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/79907")] | ||
|
|
@@ -31,6 +34,6 @@ public void StdLib_DisableSdkPath() | |
| DisableSdkPath = true, | ||
| }; | ||
|
|
||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} /optionstrict:custom /nosdkpath /out:test.exe test.vb", vbc.GenerateResponseFileContents()); | ||
| AssertEx.Equal($"/sdkpath:{RuntimeEnvironment.GetRuntimeDirectory()} @{RspFilePath} /optionstrict:custom /nosdkpath /out:test.exe test.vb", vbc.GenerateResponseFileContents()); | ||
| } | ||
| } | ||
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.
It doesn't look like we are testing scenario when an .rsp file is already specified #Closed