Skip to content
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

Set CompilerApiVersion in Microsoft.Managed.Core.targets #56460

Merged
merged 2 commits into from
Sep 22, 2021

Conversation

eerhardt
Copy link
Member

This allows other targets to understand which compiler will be used.
This is useful in the SDK targets that need figure out the version in order to pick the right analyzer assets.

Contributes to #52265

FYI - @dsplaisted. This resolves the feedback from dotnet/sdk#20793 (comment)

This allows other targets to understand which compiler will be used.
This is useful in the SDK targets that need figure out the version in order to pick the right analyzer assets.

Contributes to dotnet#52265
@eerhardt eerhardt requested a review from a team as a code owner September 16, 2021 23:33
<CompilerVersionTargetsFileContent>
<![CDATA[<Project>
<PropertyGroup>
<CompilerApiVersion>roslyn$(CompilerApiVersion)</CompilerApiVersion>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: an alternative, simpler approach could be to straight up hard-code roslyn4.0 directly into Microsoft.Managed.Core.targets. And then rely on the test that I added to break whenever we bump the Major/MinorVersion properties in the repo - reminding the person who did the bumping to update the hard-coded version.

I'm open to either approach. I'll let the owners decide which is more preferable (or suggest another alternative).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the exact approach I was concerned about, so I'm definitely in favor of doing it this way.

Outputs="$(CompilerVersionTargetsFileIntermediatePath)">

<PropertyGroup>
<CompilerApiVersion>$([System.Version]::Parse($(VersionPrefix)).Major).$([System.Version]::Parse($(VersionPrefix)).Minor)</CompilerApiVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I realize I should have brought this up sooner but shoudn't we prefix this property name with Roslyn? Basically RoslynCompilerApiVersion vs. CompilerApiVersion? Reasonable for other compilers to provide similar properties.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable for other compilers to provide similar properties.

The reason I didn't put Roslyn in property name is exactly for that reason - to support other compilers. The CompilerApiVersion version gets piped into the ResolvePackageAssets task in the dotnet/sdk:

https://github.com/dotnet/sdk/blob/500e1f61104f94753d7d84c3941d93fec2814291/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets#L267-L272

    <ResolvePackageAssets
      ProjectAssetsFile="$(ProjectAssetsFile)"
      ProjectAssetsCacheFile="$(ProjectAssetsCacheFile)"
      ProjectPath="$(MSBuildProjectFullPath)"
      ProjectLanguage="$(Language)"
      CompilerApiVersion="$(CompilerApiVersion)"

I didn't want that task to have to understand compiler-specific properties (for example needing to read both RoslynCompilerApiVersion or FSharpCompilerApiVersion).

Instead, I just made a general CompilerApiVersion which can be set to roslynX.Y or fsharpX.Y or whatever. This way, the task shouldn't need to be updated to support other compilers in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ... makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants