Adding basic package validation checks for releasing the v1 version. #7177
Conversation
Anipik
requested review from
akoeplinger,
stephentoub,
terrajobst,
ViktorHofer,
eerhardt,
ericstj,
tarekgh,
joperezr and
safern
| List<NuGetFramework> packageTargetFrameworks = package.PackageAssets.Where(t => t.AssetType != AssetType.RuntimeAsset).Select(t => t.TargetFramework).Distinct().ToList(); | ||
| if (!File.Exists(packagePath)) | ||
| { | ||
| Log.LogError($"{packagePath} does not exist. Please check the package path."); |
There was a problem hiding this comment.
User facing errors need to be in resx so that we can localize them.
There was a problem hiding this comment.
yes, thats not supported yet, thats on the to-do list.
| return false; | ||
| } | ||
|
|
||
| Helpers.ExtractPackage(PackagePath, ExtractFolder); |
There was a problem hiding this comment.
Why do we need to extract to disk? Can't we just open the package as ZipArchive and then pull out the streams as we need them? It will save unnecessary IO (write to disk only to read back to memory later on).
| } | ||
| else | ||
| { | ||
| RunApiCompat(compileTime.Path, runtime.Path); |
There was a problem hiding this comment.
The way this is done, it means it will create an assembly loader instance for each rid for the same compile time asset, meaning it will re-read the same assembly over and over again to load it. I would instead load the left assembly symbols once and re-use it to compare against it RID.
Also, I will open a card in our project to optimize for this scenario, so that there is a way to tell the differ to not recreate the tree for a side of the comparison, so that we can re-use the same tree and just build the other side and then just compare it.
| } | ||
| } | ||
|
|
||
| public void ValidateCompileAndRuntimeAgainstRelatedTfms(Package package) |
There was a problem hiding this comment.
You should probably have an abstraction for a set of comparisons. I can imagine a single package might be the source for a many different comparisons, a pair of packages can be the source for more comparisons. You may want to "plan" those, then minimize them, and decide how to best schedule them for performance.
There was a problem hiding this comment.
Also we should think of an abstraction for this validations as a public object model that we could re-use, i.e I would imagine we could offer a command line app so that people can validate their packages or even integrating it with the nuget package explorer. Something that we just pass down a Package object, and then have different public APIs that return a List of diagnostics, and then the consumer can decided to log errors to the console or display them on the UI or something like that?
| } | ||
| } | ||
|
|
||
| private void RunApiCompat(string compileTimeDll, string runtTimeDll) |
There was a problem hiding this comment.
I can imagine we Run API for many different reasons. Compile needs to be compatible with runtime, compatible TFMs, previous version of package. I'd expect us to be describing the source of the operands and why we are doing the comparison so that the user understands what's going on and why its important.
| } | ||
| } | ||
|
|
||
| public static void ExtractPackage(string filePath, string extractFolder) |
There was a problem hiding this comment.
Something we might want to consider instead of writing out files to the user's disk is to do everything via streams if possible. I could add an API to the AssemblyLoader to take a Stream instead of a file path and create the assembly symbol from the stream directly.
That might impact on memory, but might have better tradeofs than writing to disk and extracting every package.
| /// <summary> | ||
| /// Creates a package object from the nupkg package. | ||
| /// </summary> | ||
| internal class NupkgParser |
There was a problem hiding this comment.
Do we want to make this a public API and create an object model that follows the NuGet object model pattern, so that if we wanted to build a command line tool on top of it and customers can run package validation from a command line tool or for example integrating it with the nuget package explorer? cc: @clairernovotny
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Related Issue dotnet/core#5700
Validation scenarios covered
Things currently no in pr
The main purpose of this pr is to get an early feedback\ideas from the folks
Please do not review the code for apicompat as @safern will be puting up a separate pr for this.