Add a source package that can be used to connect to the roslyn build server #78986
Conversation
jjonescz
added
the
Feature - Run File
jjonescz
force-pushed
the
build-client-pkg
branch
from
f84be6c to
0f82cda
Compare
| @@ -72,7 +75,7 @@ internal static BuildRequest CreateBuildRequest( | |||
| arguments, | |||
| workingDirectory: workingDirectory, | |||
| tempDirectory: tempDirectory, | |||
| compilerHash: BuildProtocolConstants.GetCommitHash() ?? "", | |||
| compilerHash: compilerHash ?? BuildProtocolConstants.GetCommitHash() ?? "", | |||
There was a problem hiding this comment.
How is it intended for the SDK to get the compiler hash?
There was a problem hiding this comment.
Similarly to how BuildProtocolConstants.GetCommitHash would do it (look for CommitHashAttribute on a compiler dll). BuildProtocolConstants.GetCommitHash itself didn't work because this is a source package so the assembly is not the C# assembly but now I realized I can probably make that utility work by changing the assembly it looks at and then remove this parameter.
There was a problem hiding this comment.
now I realized I can probably make that utility work by changing the assembly it looks at and then remove this parameter.
I take this back, the utility is included as source in different projects and it's not possible to make it grab the CommitHash attribute from one assembly which all would reference, so probably best to keep as I have it now.
| <Compile Include="$(MSBuildThisFileDirectory)..\..\Compilers\Core\Portable\InternalUtilities\PlatformInformation.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)..\..\Compilers\Shared\BuildServerConnection.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)..\..\Compilers\Shared\NamedPipeUtil.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)..\..\Compilers\Shared\RuntimeHostInfo.cs" /> |
There was a problem hiding this comment.
It's always bugged me how this logic is spread out among so many files in different directories. Did you consider as part of this pulling it into a singe isolation directory?
There was a problem hiding this comment.
Sounds good, I can move them all to the Shared directory.
Not sure if I should move those being shared from Compilers\Core\Portable as well - especially the InternalUtilities are referenced from many other projects and there is a lot of them (63 .cs files in InternalUtilities) and I'd expect we either move all or none of them. But now the Shared folder contains mostly build server protocol related files, but with InternalUtilities moved in this nice separation would be lost.
| <PackageId>Microsoft.CodeAnalysis.BuildClient</PackageId> | ||
| <IncludeBuildOutput>false</IncludeBuildOutput> | ||
| <PackageDescription> | ||
| Package containing sources of Microsoft .NET Compiler Platform ("Roslyn") build client API. For internal use only. |
There was a problem hiding this comment.
Please add a bit how we can and will break the APIs here.
| @@ -2,6 +2,8 @@ | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
| // See the LICENSE file in the project root for more information. | |||
|
|
|||
| #nullable enable | |||
There was a problem hiding this comment.
The only intended consumers of this package are us and the sdk repo. Both of us have nullable on by default. Do we need these to be here?
There was a problem hiding this comment.
Yes, because the sources from the source package are considered to be "generated code" due to this:
and hence they need an explicit #nullable enable.
| <Compile Include="$(MSBuildThisFileDirectory)..\CommandLine\BuildProtocol.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)..\CommandLine\ConsoleUtil.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)..\CommandLine\NativeMethods.cs" /> | ||
| <Compile Include="$(MSBuildThisFileDirectory)..\CommandLine\CompilerServerLogger.cs" /> |
There was a problem hiding this comment.
Is there a reason we didn't have effectively the following instead of manually listig the files?
<Import Project="..\..\NuGet\Microsoft.CodeAnalysis.BuildClient.Package\Microsoft.CodeAnalysis.BuildClient.targets" />There was a problem hiding this comment.
I think that can be done here. But for example in VBCSCompilerCommandLine.projitems which also shares some of these files, it can't be done since there would be conflicts (because it doesn't currently share all these files and some of the types it gets from the referenced projects, maybe that could be avoided as well, but it seems to be a deep rabbit hole).
This package will be used by the SDK CLI to communicate with the build server (so
dotnet run file.cscan skip MSBuild and "run csc directly" instead when possible - note that it cannot actually start csc.exe because process start overhead would negate any optimizations from skipping MSBuild). See dotnet/sdk#48011.The set of files from the package is also used by the Replay tool to dogfood them in this repo (and to verify it is a coherent set of files).