Add BuildValidator to test-build-correctness#45593
Conversation
can be reconstructed from information in PDB
371a3fa to
250aa9f
Compare
src/Tools/BuildValidator/Program.cs
Outdated
| if (!TryLoadAssembly(file.FullName, out var assembly) || assembly is null) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
It'd be good to avoid assembly loading. The tool should be able to validate assemblies that can't be loaded on the platform the tool is running on. Also loading the file twice - here and below to PEReader.
You can use MetadataReader to read the attribute.
There was a problem hiding this comment.
Done, but I couldn't quite figure out the best way to load the custom attribute just from the MetadataReader. Will revisit on Monday, left a TODO until then.
|
Hi folks, just checking in on this? |
| return _metadataReferenceInfo; | ||
| } | ||
|
|
||
| public OutputKind GetOutputKind() => OutputKind.DynamicallyLinkedLibrary; |
There was a problem hiding this comment.
This looks incorrect. Would expect it to read the PDB here or have a issue tagged for follow up work.
There was a problem hiding this comment.
Looking into fixing this, output kind isn't one of the things we store explicitly but I believe should already be available
| _indexDirectories.Add(GetNugetCacheDirectory()); | ||
| } | ||
|
|
||
| public static DirectoryInfo GetNugetCacheDirectory() |
There was a problem hiding this comment.
I worry a bit about using the NuGet directory here. It doesn't seem strictly necessary here, it would seem that the contents of bin should be enough for this tool to operate.
Using the NuGet directory means it's possible that the validator will succeed on one machine but fail on another. That is because the NuGet direcotry contains the binaries from the totality of all restorres on the machine, not just the Roslyn one. This should be relatively safe in our CI but would like to avoid this if it's possible to do so.
There was a problem hiding this comment.
There's a lot of binaries we don't output to our bin. Here's the first 20 from my local testing. I'm not sure how to resolve these correctly otherwise? I think the idea that it's safe for our CI and scoped there should be sufficient, and hopefully if there is an issue it's easily diagnosable on a machine.
[0]: "Microsoft.Bcl.AsyncInterfaces.dll"
[1]: "Microsoft.CodeAnalysis.dll"
[2]: "Microsoft.CodeAnalysis.VisualBasic.Workspaces.dll"
[3]: "Microsoft.CodeAnalysis.Workspaces.dll"
[4]: "Microsoft.CSharp.dll"
[5]: "Microsoft.Win32.Registry.dll"
[6]: "mscorlib.dll"
[7]: "netstandard.dll"
[8]: "PresentationCore.dll"
[9]: "System.Buffers.dll"
[10]: "System.ComponentModel.Composition.dll"
[11]: "System.Configuration.dll"
[12]: "System.Core.dll"
[13]: "System.Data.DataSetExtensions.dll"
[14]: "System.Data.dll"
[15]: "System.dll"
[16]: "System.Drawing.dll"
[17]: "System.IO.Compression.FileSystem.dll"
[18]: "System.IO.dll"
[19]: "System.IO.Pipelines.dll"
|
|
||
| var files = referenceArray.Select(r => _cache[r.Mvid]); | ||
|
|
||
| var metadataReferences = files.Select(f => MetadataReference.CreateFromFile(f)).Cast<MetadataReference>().ToImmutableArray(); |
There was a problem hiding this comment.
The creation of the MetadataReference should apply the extern aliases in the associated MetadataReferenceInfo correct?
|
More feedback left to address, but pushing what I have to resolve things I've made progress on. |
Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
|
Last 3 remaining items:
|
| return assembly.Modules.Select(m => m.ModuleVersionId).ToImmutableArray(); | ||
| PEReader reader = new PEReader(stream); | ||
|
|
||
| if (reader.HasMetadata) |
There was a problem hiding this comment.
Had no idea that existed, will save me a lot of first chance exceptions in other projects.
Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
|
@ryzngard suggestion: for the open issues you listed we could create follow up issues. Basically move forward with design as is and follow up with those other issues in future PRs |
Yes, that's a great idea. I'll get to that this afternoon. I think with open issues we can't enable this as a test on CI, but it's very close |
|
Hello @ryzngard! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
BuildValidator exists to do end to end testing that binaries we produce in Roslyn can be rebuilt using embedded compilation options from the PDB.
The goal is to provide an integration test for this feature. It sets the ground work for this being abstracted into a separate tool, but that is a nongoal for this PR.
EDIT: There are follow up items that will need to be addressed before this can be on as part of CI.
#48773
#48774
#48775