Package validation #196
Package validation #196
Conversation
terrajobst
changed the title
| > version of the NuGet package (Fabrikam.Client 0.6.3) but no longer exists in | ||
| > the current version being built (0.7.0). This is a breaking change. | ||
|
|
||
| Since Armani's library is still an 0.x, which, according to SemVer, doesn't |
There was a problem hiding this comment.
Does the analyzer look at the version number? I assume not
There was a problem hiding this comment.
I assume this would be a configuration that a user would set in their project as this feature would be opt-in.
There was a problem hiding this comment.
Oh, perhaps it will not report breaking changes if they are between two packages with different major versions? If so, is there a way to force it to do so anyway (as we would for our own packages)?
There was a problem hiding this comment.
I think I would always report errors and then users can suppress messages for intentional breaks. I would be scared of having a mode that runs by default and swallows errors that might help the user avoid a breaking change and rather have them being intentional about suppressing that warning.
There was a problem hiding this comment.
That makes sense. I know that for Microsoft, we may increment the major version for a variety of reasons, such as marketing, not because it's time to break stuff.
There was a problem hiding this comment.
I thought about that. Unless I'm missing something, I don't believe we need configuration regarding the version number, but we will need an opt-in for previous version validation and we need a way to configure feed(s).
- The tool knows which version is being built.
- For each feed, the tool can request the existing versions. The previous version is the one that immediately precedes the current version in a combined sorted list.
Originally I thought about a setting for "ignore breaking changes across major version boundaries" but it seems even if you do cross a SemVer boundary you still want to make intentional breaking changes only. The tool only knows which ones are intentional if you the developer suppressed them, so I don't believe we need a SemVer setting at all.
I've added a section to the doc to cover this.
| this specific instance of the breaking change: | ||
|
|
||
| ```C# | ||
| [SuppressMessage("Interoperability", "SYSLIB1234")]] |
There was a problem hiding this comment.
Do we care about the size footprint that this may add to costumer binaries in metadata? I agree we should support disabling code this way, however should we provide a way to disable errors via an input text file or an MSBuild itemgroup? This could be modeled as tuples:
SYSLIB1234, Connect(System.String, System.TimeSpan)
or
<PackageValidation Include="SYSLIB1234" Member="Connect(System.String, System.TimeSpan)" />There was a problem hiding this comment.
Do we have precedent for designating types/members in MSBuild like this? I wonder whether it would be fragile ..
There was a problem hiding this comment.
I'm not sure; but I would imaging adding these attributes for all the APICompat errors we baseline in dotnet/runtime for example which is a pretty large codebase would not be a good option because of the metadata bloat those would cause.
There was a problem hiding this comment.
cc: @ericstj for opinions/ideas as well.
There was a problem hiding this comment.
Another option would be #pragma's that could be read by the tool, either through symbol->source mapping or read by an analyzer and shuffled over to the API compat tool through a file on the side, perhaps even the PDB (same could be done through Conditional attributes). Those could mitigate size impact on assembly. I think authoring in source is significantly better than msbuild project file, which is better than random xml/text file.
In this particular case I see the attribute is applied to the new method. That could be problematic. When analyzing metadata we wouldn't necessarily see the removal of one overload and the addition of a new one as the same thing. In the case that many overloads were added, which one should get the suppression for the removed overload? When members are completely removed, where do you put the suppression? API Compat solved this with a crude method: suppress by the full log string.
Another concern: we'd want to make sure folks aren't unnecessarily suppressing warnings. Folks might also want to have many runs of the tool (eg: once against previous major version, once against previous minor). Where would they put the different suppressions? Also, if they were only running against previous version, would we ask them to remove all the suppressions once they passed the single version that introduced the breaking change?
There was a problem hiding this comment.
Another option would be #pragma's that could be read by the tool, either through symbol->source mapping or read by an analyzer and shuffled over to the API compat tool through a file on the side, perhaps even the PDB (same could be done through Conditional attributes). Those could mitigate size impact on assembly. I think authoring in source is significantly better than msbuild project file, which is better than random xml/text file.
Wouldn't that mean that if we wanted to run against the previos version of the package which had it's own APICompat suppressions we would need to run against the symbols package if the PDBs are not part of the package that we downloaded?
Also we would need to come up with our own #pragma format that included RID/TFM/SemVer if you wanted to suppress for a specifc major version but not for others? Or if you wanted to have a suppression for just one RID but not for the others, etc?
There was a problem hiding this comment.
Wouldn't that mean that if we wanted to run against the previos version of the package which had it's own APICompat suppressions we would need to run against the symbols package if the PDBs are not part of the package that we downloaded?
Understanding if these suppressions need to travel with the binary itself is an important distinction. That's not clear to me today: in all scenarios I've seen, right-hand-side is being built. If we want to support otherwise then I agree we should have a reliable persistent storage location.
Also we would need to come up with our own #pragma format that included RID/TFM/SemVer if you wanted to suppress for a specifc major version but not for others? Or if you wanted to have a suppression for just one RID but not for the others, etc?
Source already has conditional syntax (conditions in project file, #ifdef in source). For applying suppressions to right-hand-side I think we should use that. Constraining the suppression based on what's on the left-hand-side could be interesting. I think we've mentioned a few different interesting variables: version, TFM, RID. What do we think these constraints might help us with?
For version my thought was that it would allow us to "permit" old suppressions to remain if the version on the left-hand-side was newer than that being analyzed. This way we wouldn't force people to delete suppressions the release after they made the breaking change (they still could if they wanted).
I'm not sure if it's so interesting for TFM/RID: if you are going to permit a break you typically are OK with that break from any source on the left-hand-side. To handle the "unused" case here, we could look at all compat runs in a batch and only raise an unused warning if none of them required the suppression (and the version rule didn't apply).
There was a problem hiding this comment.
Yeah I can't think of a scenario where left would need to carry suppressions over, but maybe there is one.
Also, something that might be interesting would be that we are running APICompat after pack (I don't know if the opt-in against a previous version would be after build or after pack, I would guess after pack so that we can actually do package validation againt it as well not just API Compat?). Any way when running after pack it would be trickier to get the PDBs as that happens in the outer build where there is no RID/TFM context, I know we could figure out the PDBs paths with some targets and stuff, but how ideal would that be?
What I'm trying to say is how ideal is depending on PDBs as well?
Also something to consider is how are we going to baseline errors for Package Validation itself, like package dependencies errors and those kind of errors? Or I guess those shouldn't be allowed to be disabled.
There was a problem hiding this comment.
Just brainstorming existing files that might already flow through the process where we could stash metadata. PDBs definitely have some downsides. Assembly metadata is, of course, easier. Was just reading our docs on suppress message and found this that addresses metadata-bloat:
The SuppressMessageAttribute attribute is a conditional attribute, which is included in the IL metadata of your managed code assembly, only if the CODE_ANALYSIS compilation symbol is defined at compile time.
You should not use in-source suppressions on release builds, to prevent shipping the in-source suppression metadata accidentally. Additionally, because of the processing cost of in-source suppression, the performance of your application can be degraded.
Maybe if we use it that way it makes it OK? Folks would need to pack debug builds to have it work.
Also something to consider is how are we going to baseline errors for Package Validation itself, like package dependencies errors and those kind of errors? Or I guess those shouldn't be allowed to be disabled.
We should also have a path for suppressions that isn't exclusively assembly metadata. NoWarn, but also more granular suppressions. We'd need to think about all the encoding we'd want to do on those.
There was a problem hiding this comment.
Maybe if we use it that way it makes it OK? Folks would need to pack debug builds to have it work.
That would make me a little nervous though, as not a lot of PR/CI validation builds run dotnet pack on Debug builds, I don't know how common that practice is? And I would guess release pipelines always pack on Release configuration, so is a behavior where people can only suppress messages on one configuration but not in the other? That could loose some fidelity as our tooling would only be running on Debug builds that run pack.
Also, if we are going to do suppressions on source as our first class mode to suppress errors I think we should provide a fixer in the near future...
| unintentional breaking changes. This is achieved by productizing our internal | ||
| tooling that we're planning to use ourselves as well. | ||
|
|
||
| ## Scenarios and User Experience |
There was a problem hiding this comment.
For clarity, am I understanding correctly that this tool runs on exactly one single package at a time?
Eg., it doesn't need to check referenced packages; nor does it attempt to compare two versions of the same package?
There was a problem hiding this comment.
Oh never mind I see the package-to-package scenario. Still it would be good to note whether it ever looks at any other packages other than the 1 or 2 supplied
There was a problem hiding this comment.
does it attempt to compare two versions of the same package
it will have the functionality to compare with a previous version of the package.
it doesn't need to check referenced packages;
we will run few checks on the package dependencies.
| unintentional breaking changes. This is achieved by productizing our internal | ||
| tooling that we're planning to use ourselves as well. | ||
|
|
||
| ## Scenarios and User Experience |
There was a problem hiding this comment.
shoud we add a user case for validation across different rids ?
There was a problem hiding this comment.
Happy to, but could you provide a rough summary of what we'd flag?
| @@ -0,0 +1,236 @@ | |||
| # Package Validation | |||
There was a problem hiding this comment.
On my first read, I confused "Package Validation" with nuget package verification.
There was a problem hiding this comment.
Perhaps another name makes sense here since many users confuse the verification feature with validation already.
|
|
||
| I don't believe we need to know the previous version. The previous version would | ||
| be defined on a per-feed basis. In any feed, the previous version is the one | ||
| that immediately precedes the version being built. |
There was a problem hiding this comment.
@terrajobst I only have the question around baselining a lot of errors and if we should support more than just the attributes. Other than that, seems good to mark as ready for a broader review?
| be defined on a per-feed basis. In any feed, the previous version is the one | ||
| that immediately precedes the version being built. | ||
|
|
||
| ***OPEN QUESTION**: Do we need a SemVer setting? Originally I thought about a |
There was a problem hiding this comment.
After reading the proposal, this tool sounds like it is fixing a case where authors do not respect SemVer & should be warned that they are doing so.
Perhaps the tool should be somewhat "SemVer aware" so that it can assess the potential disruption the author can cause by not incrementing the respective version number.
"You made an incompatible API change detected by our tool, you should bump your major version."
"You made a backwards compatible API change detected by our tool, you should bump your minor version."
This might help empower users to embrace SemVer further with actionable steps to take or ignore.
| * Performance should be good so that basic package validation can be on by | ||
| default (when the project generates a package), but it must be possible to | ||
| turn it off as well | ||
| * Comparing to previous version should be opt-in because it requires downloading |
There was a problem hiding this comment.
How would the tool work without comparing to a previous version?
There was a problem hiding this comment.
It would be running as part of the dotnet pack step and would make sure that the produced package is API Compatible (in between compile and runtime assets), well formed, closure complete, etc.
There was a problem hiding this comment.
@safern Will there be any flag or metadata that one can assess the <ValidatePackageAgainstPreviousVersion> property was used at pack time in the resulting package?
| turn it off as well | ||
| * Comparing to previous version should be opt-in because it requires downloading | ||
| additional files | ||
| * Developers need to be able to make intentional breaking changes while still |
There was a problem hiding this comment.
Maybe if semver is not detected that intentional breaking changes can be made? i.e. 0.Y.Z
Otherwise if semver detected, they can choose to ignore / suppress the warnings/errors? i.e. X.Y.Z where >= 1.0.0
No description provided.