Better rebuild logging #51852
Better rebuild logging #51852
Conversation
This moves our output logging to include a target framework directory. Before we weren't doing this and we ended up with a race condition when the same project failed to rebuild in multiple target frameworks. Cleaned up the handling of failed `CompilationDiff` instances by turning it into a adhoc-union
jaredpar
added
Area-Compilers
New Language Feature - Rebuild
| } | ||
| else | ||
| { | ||
| var originalBytes = File.ReadAllBytes(originalBinaryPath.FullName); | ||
| var originalBytes = File.ReadAllBytes(assemblyInfo.FilePath); |
There was a problem hiding this comment.
should we add a note that we want to avoid doing I/O here? e.g. eventually we need to make the public caller pass in a PE stream or pass in the assembly fully read into memory.
There was a problem hiding this comment.
no need to change the PR for this--but this is a good moment to decide if we need to file an issue
There was a problem hiding this comment.
Suddenly it occurs to me that we're in the CLI tool's area now, so it's not as much of a concern--but if we end up pulling parts of this over into the library side to help users diagnose rebuild failures, I think we'll have to keep an eye out for this sort of thing.
There was a problem hiding this comment.
Agree we need to keep an eye out for this. Really need to put a hard stop on anything like this going into the library.
| @@ -278,25 +288,30 @@ private static bool ValidateFiles(IEnumerable<FileInfo> originalBinaries, Option | |||
| try | |||
| { | |||
| // Find the embedded pdb | |||
| using var originalBinaryStream = originalBinary.OpenRead(); | |||
| using var originalBinaryStream = File.OpenRead(assemblyInfo.FilePath); | |||
There was a problem hiding this comment.
nit: feels like this line doesn't need to change if the declaration of originalBinary is moved up.
| fixed (byte* ptr = _rebuildPortableExecutableBytes) | ||
| { | ||
| using var originalPeReader = new PEReader(ptr, _originalPortableExecutableBytes.Length); | ||
| using var rebuildPeReader = new PEReader(ptr, _rebuildPortableExecutableBytes.Length); |
There was a problem hiding this comment.
The two readers are reading from the same ptr buffer? #Closed
There was a problem hiding this comment.
Well that would explain the one head scratcher I was looking at 😦
src/Tools/BuildValidator/Program.cs
Outdated
| sources, | ||
| metadataReferences); | ||
| if (compilation is null) | ||
| Compilation? compilation = null; |
There was a problem hiding this comment.
Compilation? compilation = null [](start = 16, length = 31)
Minor: Looks like this can be declared non-nullable with no default value: Compilation compilation; #Closed
|
Thanks. Responded to feedback. |
| OriginalPath = originalPath; | ||
| get | ||
| { | ||
| EnsureRebuildReslt(RebuildResult.MiscError); |
There was a problem hiding this comment.
Reslt [](start = 29, length = 5)
Typo?
There was a problem hiding this comment.
My typos are eternal ....
|
Integration failures are flaky tests. |
ghost
added this to the 
This moves our output logging to include a target framework directory.
Before we weren't doing this and we ended up with a race condition when
the same project failed to rebuild in multiple target frameworks.
Cleaned up the handling of failed
CompilationDiffinstances by turningit into a adhoc-union