Roslyn non-source-only build fixes #72360
Conversation
These changes make it possible to build roslyn in non-source-only modes in the VMR: - Remove some vertical build (non-source-only) exclusions that were added during the PoC phase of the VMR - When packing the language server nuget package, Pack only the rid that the current vertical is building. - Mark a couple benchmark projects as IsTestProject. Tests are excluded by default when building the VMR - Mark a couple of test utility projects as test utility projects. When not building tests, these projects are excluded from the build.
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
| <RuntimeIdentifiers>win-x64;win-x86;win-arm64;linux-x64;linux-arm64;linux-musl-x64;linux-musl-arm64;osx-x64;osx-arm64</RuntimeIdentifiers> | ||
| <!-- List of runtime identifiers that we want to publish an executable for. --> | ||
| <!-- When building a VMR vertical, we don't need to pack everything. Just pack the passed in TargetRid. --> | ||
| <RuntimeIdentifiers Condition="'$(TargetRid)' != ''">$(TargetRid)</RuntimeIdentifiers> |
There was a problem hiding this comment.
Where do we document TargetRid? Think of the exercise we're going through with RoslynPortableRuntimeIdentifiers right now. We're looking at that, searching for it in the code, can't find any references and are assuming "it's probably okay to delete this item". A rationale developer could apply the exact same logic to $(TargetRid) uses.
There was a problem hiding this comment.
Updated comments and added more docs for TargetRid (currently in PR)
| @@ -10,6 +10,7 @@ | |||
| <LangVersion>11</LangVersion> | |||
| <ProduceReferenceAssembly>False</ProduceReferenceAssembly> | |||
| <NoWarn>$(NoWarn);NETSDK1206</NoWarn> | |||
| <IsTestProject>true</IsTestProject> | |||
There was a problem hiding this comment.
These aren't correct. Will revert.
There was a problem hiding this comment.
Change this to ExcludeFromDotNetBuild when not building tests (no effect outside of VMR builds) and added pointers to docs.
...ageServer/Microsoft.CodeAnalysis.LanguageServer/Microsoft.CodeAnalysis.LanguageServer.csproj
Outdated
Show resolved
Hide resolved
| So, when test projects are excluded, exclude this benchmark project. | ||
| See https://github.com/dotnet/arcade/blob/main/Documentation/UnifiedBuild/Unified-Build-Controls.md for information | ||
| on controls. --> | ||
| <ExcludeFromDotNetBuild Condition="'$(DotNetBuildTests)' != 'true'">true</ExcludeFromDotNetBuild> |
There was a problem hiding this comment.
Wondering if there should be some <IsBenchmarkProject> flag and the logic to exclude those from VMR would be shared somewhere like Directory.Build.props - then we would not need to copy the long comment to each benchmark project.
There was a problem hiding this comment.
Yes, Arcade has a flag for identifying performance test projects: https://github.com/dotnet/arcade/blob/87d89025bdd8827c016e4083660d31f497670e5c/src/Microsoft.DotNet.Arcade.Sdk/tools/Tests.props#L5
Problem with that is that it assumes that those projects are run with the old xunit.performance library which hopefully no one uses anymore. So Arcade would need a bit more work before performance test projects can set the <IsPerformanceTestProject>true</IsPerformanceTestProject> property.
There was a problem hiding this comment.
okay, I meant more like a flag only local to the roslyn repo to share this logic instead of copy-pasting it
There was a problem hiding this comment.
@jjonescz I looked into doing this. There's no place to insert such a flag without duplicating the arcade logic around project exclusion. This is because the common logic that processes project exclusion is imported via a .NET SDK hook prior to any roslyn common targets files (e.g. eng/Imports.BeforeArcade.targets).
…ver/Microsoft.CodeAnalysis.LanguageServer.csproj Co-authored-by: Viktor Hofer <[email protected]>
|
@jjonescz I may need some input on the integration test failure. It doesn't seem to be related, but I want to make sure. |
|
Yes, that's known flaky test. Should be fixed now though via #72391, so I will try rerunning it (it should run with latest main merged in automatically I think); or you can get this force-merged perhaps |
Retry doesn't re-merge. /azp run |
These changes make it possible to build roslyn in non-source-only modes in the VMR: