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
| @@ -337,8 +337,12 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| </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 |
| @@ -336,9 +336,15 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| </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.
Fixes #50419.
Closes #23533 (the tests have been re-enabled).