Reference ILLink analyzer matching ILLink version #32045
Conversation
And remove bundled ILLink analyzer
dotnet-issue-labeler
bot
added
Area-ILLink
untriaged
| var analyzerPackage = new TaskItem("Microsoft.NET.ILLink.Analyzers"); | ||
| analyzerPackage.SetMetadata(MetadataKeys.Version, packVersion); | ||
| implicitPackageReferences.Add(analyzerPackage); |
There was a problem hiding this comment.
Just curious: where do we download this from? It seems it's not on NuGet... is the SDK adding some other public feeds for this?
There was a problem hiding this comment.
I think it's downloaded from NuGet: https://www.nuget.org/packages/Microsoft.NET.ILLink.Analyzers/
For internal builds the specific version would be available on an internal feed.
There was a problem hiding this comment.
That package was submitted by mistake - it only has one 8.0 preview version, it definitely doesn't contain downlevel versions.
There was a problem hiding this comment.
I'll ask for it to be uploaded to nuget.
| <Dependency Name="Microsoft.NET.ILLink.Analyzers" Version="8.0.100-1.23067.1"> | ||
| <Uri>https://github.com/dotnet/linker</Uri> | ||
| <Sha>c790896f128957acd2999208f44f09ae1e826c8c</Sha> | ||
| </Dependency> |
There was a problem hiding this comment.
Should the corresponding version number be removed from Versions.props? Otherwise I believe it will still be there and available for use, but won't ever get updated.
There was a problem hiding this comment.
It looks like we were using the ILLink.Tasks package version, and this Version.Details.xml version was never used. I removed the ILLink.Tasks packagereference and cleaned up some left-over analyzer layout logic that was using it.
I left in the ILLink.Tasks entry in Versions.props and Version.Details.xml even though these are technically unused. We test with an ILLink.Tasks version matching MicrosoftNETCoreAppRefPackageVersion, which should always be the same, but it kind of seems nice to make the dependency explicit at least in Versions.props. Let me know if you'd prefer me to remove it and I will.
| var analyzerPackage = new TaskItem("Microsoft.NET.ILLink.Analyzers"); | ||
| analyzerPackage.SetMetadata(MetadataKeys.Version, packVersion); | ||
| implicitPackageReferences.Add(analyzerPackage); |
There was a problem hiding this comment.
I think it's downloaded from NuGet: https://www.nuget.org/packages/Microsoft.NET.ILLink.Analyzers/
For internal builds the specific version would be available on an internal feed.
|
This is failing in a test that checks that incremental publish of a single-file doesn't re-generate the single-file if there are no changes. It is caused by #28661. |
| <_IsTrimmingEnabled Condition="'$(_IsTrimmingEnabled)' == '' And ('$(PublishTrimmed)' == 'true' Or '$(IsTrimmable)' == 'true')">true</_IsTrimmingEnabled> | ||
| <_IsTrimmingEnabled Condition="'$(_IsTrimmingEnabled)' == '' And ( | ||
| '$(PublishTrimmed)' == 'true' Or | ||
| '$(PublishSingleFile)' == 'true' Or |
There was a problem hiding this comment.
Can we rename _IsTrimmingEnabled? It doesn't seem like a good name anymore, if we are setting it to true when I only set PublishSingleFile.
There was a problem hiding this comment.
dotnet/runtime uses that property already. @sbomer can you please take care of updating it when these changes are being consumed? https://github.com/dotnet/runtime/blob/98dd1e83ba8b17299af9d3d78000f8a9c000a12e/eng/illink.targets#L5C1-L6
|
@sbomer - This causes a failure when using the SDK containing these changes to build the runtime repo. This is affecting source-build. Can you investigate please? I have this issue logged with details: dotnet/source-build#3490 |
Before net8.0, the illink analyzer shipped as a separate package from ILLink.Tasks. #32045 added logic to reference the separate package for tfms less than net8.0. This change tweaks the logic to be based on the package version resolved from KnownILLinkPack, rather than the tfm. This makes it possible to define KnownILLinkPack to pull in the latest analyzer, as a workaround when targeting unsupported TFMs. The scenario is still considered unsupported, but this at least makes it possible to work around. A new testcase shows the steps necessary to use the latest illink analyzer on a netstandard2.x project.
Up to .NET 7, the analyzers versioned with the SDK (so you would always get the latest analyzers). This allowed using the analyzers (and related options which enable the analyzers) even on older TFMs. dotnet#32045 changed to versioning the analyzers with the target framework, but still allowed use of the analyzers on target frameworks where the .NET 7 ILLink pack was made available for backwards compatibility. This change removes such backwards compat by treating the analyzers as unsupported for target frameworks before the framework was annotated with the relevant attributes. PublishTrimmed/PublishSingleFile/PublishAot used to enable the corresponding analyzers. With this change, we now need to avoid failing PublishTrimmed/PublishSingleFile for older TFMs that supported these publish options, but don't support the analyzers.
Up to .NET 7, the analyzers versioned with the SDK (so you would always get the latest analyzers). This allowed using the analyzers (and related options which enable the analyzers) even on older TFMs. #32045 changed to versioning the analyzers with the target framework, but still allowed use of the analyzers on target frameworks where the .NET 7 ILLink pack was made available for backwards compatibility. This change removes such backwards compat by treating the analyzers as unsupported for target frameworks before the framework was annotated with the relevant attributes. This increases the scope of the warning added in #29441 and #34077 to also warn for these unsupported TFMs. The first commit adds testcases to show the old behavior. The second commit implements the new behavior and shows the changes to these testcases. PublishTrimmed/PublishSingleFile/PublishAot used to enable the corresponding analyzers. With this change, we now need to avoid failing PublishTrimmed/PublishSingleFile for older TFMs that supported these publish options, but don't support the analyzers. This change handles that by only defaulting the `Enable*Analyzer` settings based on the publish settings for TFMs where the analyzers are known to be supported.
Fixes dotnet/runtime#84119.
We were already referencing the analyzer from the ILLink package on net8.0, so there we only needed to remove the bundled analyzer to fix dotnet/runtime#84119.
For older TFMs, we currently use the latest analyzer that's bundled with the SDK. I replaced this with logic to use a packagreference, matching the version of ILLink. Note that with this change, when targeting downlevel TFMs the 8.0 SDK will use the 7.0 analyzer instead of the 8.0 analyzer that we use today.