Public api analyzer change #8116
Conversation
I think you're looking for this doc (next to that one in the TOC): https://learn.microsoft.com/dotnet/fundamentals/package-validation/baseline-version-validator |
|
Looking through the documentation, it seemed to say I should include a previous version of the package. Since all the examples I saw were three-part version numbers, I'd say 17.4.0. Does that sound appropriate? We should add this to the list of things to update in the "Release 17.x" issues. |
|
Looking at this, it seems like it should be ok once 17.4 is publicly released, which should be fairly soon, so I'm happy waiting 'til then. |
Forgind
force-pushed
the
public-api-change
branch
from
df5d6de to
be2fc72
Compare
Forgind
force-pushed
the
public-api-change
branch
from
be2fc72 to
8046240
Compare
* Drop the net35 asset from the StringTools package As discussed offline, the StringTools package is currently not authored correctly as the StringTools assembly is differently named between target frameworks. More precisely, the net35 assembly is named differently than the rest of the builds: Microsoft.NET.StringTools.net35.dll. Reason for that is that this assembly is binplaced into a common folder with another tfm build of the same library. By dropping the net35 asset from the package, assembly FileNotFoundExceptions are avoided. Contributes to #8116(#8116)
|
/azp run msbuild-pr |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ViktorHofer
left a comment
ViktorHofer
left a comment
There was a problem hiding this comment.
Is the set of projects for which you enabled package validation the full set of packable projects in the repo? If so, then you can move the EnablePackageValidation property to a central location.
I would assume our unit tests are not packable, but our samples are, so I could centralize it then turn it off in our Samples folder, but there aren't too many projects, so I think this is roughly as good. |
| <DiagnosticId>PKV004</DiagnosticId> | ||
| <Target>Xamarin.PlayStationVita,Version=v0.0</Target> |
There was a problem hiding this comment.
What do these mean, and why are they necessary? Is it an artifact of moving from net6 to net7 between 17.3 and 17.4? If so should we bump the baseline to 17.4?
There was a problem hiding this comment.
Yes good catch! We should update the baseline to 17.4 to get cleaner baseline files.
There was a problem hiding this comment.
This is an indication that the package supports netstandard2.0 as a contract but doesn't offer compatible runtime assemblies. It lists the set of platforms that the packages doesn't at run time but at compile time.
In dotnet/runtime we would call this a packaging bug as offering a contract without an implementation results in a poor experience at consumption time. Developers would be able to compile to but fail during runtime with an Exception. In dotnet/runtime we intentionally offer PNSE assemblies that throw a PlatformNotSupportedExceptions with a localized message.
There was a problem hiding this comment.
Ah, great. I'll push a commit with some comments explaining our reasoning behind this decision.
There was a problem hiding this comment.
We stopped using ref in runtime packages because there were too many scenarios that didn't work well with it. There are old design time tools that try to use reflection (which loads for execution by default) to load references - or actually try to run them.
It's also not a great user experience to have a ref with no lib since it would mean that the only indication someone got of missing support is a runtime FileNotFoundException at an unpredictable point when the JIT decides to load the assembly.
The pattern we follow in runtime is to include a PNSE implementation assembly in lib/netstandard2.0 that can be used for reference. We use SupportedOSPlatform attributes to help message when things won't work on other platforms.
Aside: the number of errors we emit here is larger than it needs to be. We could just emit a single error about the most specific TFM missing (netstandard2.0) and then could provide the other TFMs in the error message. This might make the error less noisy for the customer and thus more likely to be understood.
There was a problem hiding this comment.
The pattern we follow in runtime is to include a PNSE implementation assembly in lib/netstandard2.0 that can be used for reference.
I think it would make sense for us to do this for all TFs, since to use MSBuild APIs you must run within the context of an installed VS or SDK, so the "normal library" versions of everything are useless. However, that's a build and packaging problem I don't think we're willing to tackle at the moment.
There was a problem hiding this comment.
Aside: the number of errors we emit here is larger than it needs to be. We could just emit a single error about the most specific TFM missing (netstandard2.0) and then could provide the other TFMs in the error message. This might make the error less noisy for the customer and thus more likely to be understood.
I do think that would have helped me.
eng/Versions.props
Outdated
| <Project> | ||
| <PropertyGroup> | ||
| <VersionPrefix>17.5.0</VersionPrefix> | ||
| <PackageValidationBaselineVersion>17.3.2</PackageValidationBaselineVersion> |
There was a problem hiding this comment.
Ideally, bumping this version should be part of rebranding the repo to the next version. Do you have documentation that should be updated?
There was a problem hiding this comment.
We generally rebrand main before the public push of the release branch. So I think this'll need to be two-phase: at rebrand time, set the baseline to the current prerelease package, then after release update the baseline to the final public release.
Added that info to #8246 (we'll automate the creation and update of these checklists Someday™️)
| <!-- For ease of logging the "not supported on Core" message, this task is a | ||
| TaskExtension on netstandard/netcore. Since the type is sealed there, | ||
| that shouldn't cause any implementation problems since no one can derive | ||
| from it and try to call TaskExtension.Log. --> |
There was a problem hiding this comment.
I would appreciate a reasonability check on this.
There was a problem hiding this comment.
That's something we've done on several other tasks as well, right? I think it seems reasonable to do it here as well.
There was a problem hiding this comment.
We didn't have any other instances of this error, right? I think the other not-supported-on-Core tasks may have already had easy access to a localized Log mechanism.
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
I'm wondering about how we're planning to update CompatibilitySuppressions.xml files. When I wanted to update them for this, I just deleted them and remade them from scratch, but if you were to do that now, we'd lose comments. It'd be nice to be able to remove stuff as relevant, though...
| <!-- For ease of logging the "not supported on Core" message, this task is a | ||
| TaskExtension on netstandard/netcore. Since the type is sealed there, | ||
| that shouldn't cause any implementation problems since no one can derive | ||
| from it and try to call TaskExtension.Log. --> |
There was a problem hiding this comment.
That's something we've done on several other tasks as well, right? I think it seems reasonable to do it here as well.
Great question! We have two issues filed to smoothen out the suppression file re-generation:
Until those are implemented you will need to delete the existing suppression files, generate them and then manually copy over the comments. I can take a look if I get a chance to implement one of these over the holidays when it's usually quieter. As ApiCompat also ships via nuget packages, it's easily possible to depend on newer features than what's available in the SDK. cc @smasher164 |
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
I'm glad to hear that improvements are coming, but the manually-copy-across flow is not so bad that it's blocking, IMO.
Forgind
added
the
merge-when-branch-open
5 participants
Fixes-ish #7903
Context
Our previous analyzer is not helpful due to #7903. This switches to using a more modern api analyzer.
Notes
The note that inspired this suggested including a version as well, but the documentation I found didn't mention a version.