Set RoslynAssembliesPath #50433
Set RoslynAssembliesPath #50433
Conversation
test/Microsoft.DotNet.ApiCompat.IntegrationTests/Task/ValidatePackageTargetIntegrationTests.cs
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
| <PropertyGroup><RoslynCompilerType>Framework</RoslynCompilerType></PropertyGroup> | ||
| </Otherwise> | ||
| </Choose> | ||
|
|
There was a problem hiding this comment.
Should we set RoslynAssembliesPath when RoslynCompilerType is not Core as well? That way we establish this as a general contract provided by roslyn/SDK. It's possible that others may wish to use that property.
There was a problem hiding this comment.
Definitely, that was my intention, thanks.
fwiw, the re-enabled tests would catch the bug (i.e., they fail like in the issue if the product fix is reverted) |
|
@jaredpar PTAL |
| </Otherwise> | ||
| </Choose> | ||
|
|
||
| <!-- NOTE: Compiler toolset packages should set the following properties on their own, hence we set them only for RoslynCompilerType being Core or Framework. --> |
There was a problem hiding this comment.
📝 I opened a follow-up in roslyn to set RoslynAssembliesPath by the toolset packages as well: dotnet/roslyn#80025
| The SDK also sets the following properties: | ||
| - `RoslynTargetsPath` to the directory path of the roslyn tasks assembly. This property is used by some targets | ||
| but it should be avoided if possible because the tasks assembly name can change as well, not just the directory path. | ||
| - `RoslynAssembliesPath` to the directory path of other roslyn assemblies (like `Microsoft.CodeAnalysis.dll`). |
There was a problem hiding this comment.
@ericstj this value can point to assemblies that target net472 or .NET core. Will the API compat tool be okay with both of these?
There was a problem hiding this comment.
I would expect it to point to the correct framework for the MSBuild host process. So if MSBuild is running as netfx, point to netfx assemblies. If MSBuild is running as core, point to core assemblies.
If you think there is a scenario for cross-framework execution (IE: if MSBuild might allow a core-taskhost in netfx build process the future) then we should also have specific properties that always point at core and netfx.
There was a problem hiding this comment.
Currently the property should point to the matching TFM assemblies. I can clarify the docs.
There was a problem hiding this comment.
Interesting. Why did you pick this? My intuition is that RoslynAssembliesPath would point to the assemblies that would be used by the build task.
There was a problem hiding this comment.
I picked it so that ApiCompat (currently the only consumer of RoslynAssembliesPath) works. And I would expect other consumers behave similarly in this regard - it's not possible to load netcore MS.CA.dll from custom netfx build task; our bridge compiler build task can work only because it starts an executable; if it loaded roslyn as a library it wouldn't work either. And I assume people will use RoslynAssembliesPath for the library DLLs, not the executables.
There was a problem hiding this comment.
Gotcha. I do wonder if we're going to have to come back here and add a property that always points to the .NET core version of the assemblies. Given how much we're pushing towards .NET core based tools, guessing there will be scenarios where a part of the build needs to load the.NET core version of Roslyn assemblies even when built via msbuild
There was a problem hiding this comment.
I can go ahead and add a property for core-only roslyn assemblies even now.
|
@jaredpar for another look, thanks |
| The SDK also sets `RoslynTargetsPath` to the directory path of the roslyn tasks assembly. This property is used by some targets | ||
| but it should be avoided if possible because the tasks assembly name can change as well, not just the directory path. | ||
| The SDK also sets the following properties: | ||
| - `RoslynTargetsPath` to the directory path of the roslyn tasks assembly. This property is used by some targets |
There was a problem hiding this comment.
Can we add a note that even though this is called the _targets _path it's really the tasks path. It's called targets because of a historical quirk but it's very possible for it to point to a directory that contains no targets.
5 participants
Fixes #50419.
Closes #23533 (the tests have been re-enabled).