Visual Basic rebuild provenance work#51479
Conversation
|
@RikkiGibson, @dotnet/roslyn-compiler PTAL |
| public const string Unsafe = "unsafe"; | ||
| public const string Nullable = "nullable"; | ||
| public const string Define = "define"; | ||
| public const string Strict = "strict"; |
There was a problem hiding this comment.
Yes this is a change from 16.9 but at the same time 16.9 is not a viable release for VB provenance. I'd rather accept that and use 16.10 to get the options written out in a consistent manner.
|
|
||
| var assemblyName = Path.GetFileNameWithoutExtension(originalBinaryPath.Name); | ||
| var assemblyDebugPath = Path.Combine(debugPath, assemblyName); | ||
| fixed (byte* ptr = rebuildBytes) |
There was a problem hiding this comment.
It may be easier to review this section by just looking at the new code vs. the diff. This essentially attempts to streamline how we write out build artifacts. I wanted to have a single path for writing build artifacts that we just call into twice.
| PdbMdvFilePath: Path.Combine(outputPath, assemblyName + ".pdb.mdv"), | ||
| ILFilePath: Path.Combine(outputPath, assemblyName + ".il"), | ||
| PdbXmlFilePath: Path.Combine(outputPath, assemblyName + ".pdb.xml"), | ||
| CustomDataFilePath: Path.Combine(outputPath, "custom-data.txt")); |
There was a problem hiding this comment.
should this also be prefixed with the assembly name?
There was a problem hiding this comment.
Didn't seem necessary here. It's only one per original / rebuild and the assembly name is in the grand parent directory so felt redundant to add. Don't feel strongly about this though.
| options: pdbToXmlOptions, | ||
| methodName: null); | ||
|
|
||
| Process.Start(@"C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.8 Tools\ildasm.exe", $@"{assemblyFilePath} /all /out={buildDataFiles.ILFilePath}").WaitForExit(); |
There was a problem hiding this comment.
randomly wondering: would ProcessStartInfo.CreateNoWindow let us prevent ildasm from popping up and stealing focus when this tool is running in the background.
| internal static MetadataReader GetEmbeddedPdbMetadataReader(this PEReader peReader) | ||
| { | ||
| var entry = peReader.ReadDebugDirectory().Single(x => x.Type == DebugDirectoryEntryType.EmbeddedPortablePdb); | ||
| var provider = peReader.ReadEmbeddedPortablePdbDebugDirectoryData(entry); |
There was a problem hiding this comment.
Is this change based on the rationale that we only care about comparing the PDBs if they are embedded, and if the pdb is instead produced as a separate file, we don't care whether it's the same as the original?
There was a problem hiding this comment.
This change was motivated by the idea that this tool will eventually be a library and the library shouldn't be going to the disk for basic information like getting a MetadataReader. There are essentially two cases for the rebuild scenario when it comes to PDB production:
- The PDB is embedded in which case this method can get a
MetadatReaderto it without going to disk - The PDB is not embedded in which case we have a
Streamfrom which we can produce aMetadataReader
This is solving the first case.
| src\Analyzers\VisualBasic\Analyzers\VisualBasicAnalyzers.projitems*{2531a8c4-97dd-47bc-a79c-b7846051e137}*SharedItemsImports = 5 | ||
| src\Workspaces\SharedUtilitiesAndExtensions\Compiler\VisualBasic\VisualBasicCompilerExtensions.projitems*{2531a8c4-97dd-47bc-a79c-b7846051e137}*SharedItemsImports = 5 | ||
| src\Analyzers\Core\Analyzers\Analyzers.projitems*{275812ee-dedb-4232-9439-91c9757d2ae4}*SharedItemsImports = 5 | ||
| src\Dependencies\Collections\Microsoft.CodeAnalysis.Collections.projitems*{275812ee-dedb-4232-9439-91c9757d2ae4}*SharedItemsImports = 5 |
There was a problem hiding this comment.
I don't understand this change. Especially since this path appears in this section maybe 4 times. Is this fine?
There was a problem hiding this comment.
I don't understand it either. If i remove VS adds it back though 🤷♀️
Overall it's safe though. It's a VS experience artifact, not a build time one.
| using System.Linq; | ||
| using System.Reflection.Metadata; | ||
| using System.Reflection.PortableExecutable; | ||
| using System.Runtime.CompilerServices; |
There was a problem hiding this comment.
| using System.Runtime.CompilerServices; |
There was a problem hiding this comment.
I do not think you introduced usages of types in this namespace.
There was a problem hiding this comment.
Most of these weren't needed. Deleted a bunch locally.
| public const string SourceFileCount = "source-file-count"; | ||
| public const string EmbedRuntime = "embed-runtime"; | ||
| public const string GlobalNamespaces = "global-namespaces"; | ||
| public const string RootNamespaces = "root-namespace"; |
There was a problem hiding this comment.
RootNamespaces [](start = 28, length = 14)
Should this be "RootNamespace" (singular)?
| bool? OptionToBool(string option) => pdbCompilationOptions.TryGetUniqueOption(option, out var value) ? ToBool(value) : null; | ||
| T? OptionToEnum<T>(string option) where T : struct => pdbCompilationOptions.TryGetUniqueOption(option, out var value) ? ToEnum<T>(value) : null; | ||
| bool? ToBool(string value) => bool.TryParse(value, out var boolValue) ? boolValue : null; | ||
| T? ToEnum<T>(string value) where T : struct => Enum.TryParse<T>(value, out var enumValue) ? enumValue : null; |
There was a problem hiding this comment.
T? [](start = 12, length = 2)
static
| using var pdbXmlStream = File.Create(buildDataFiles.PdbXmlFilePath); | ||
| PdbToXmlConverter.ToXml( | ||
| new StreamWriter(pdbXmlStream), | ||
| pdbStream: new UnmanagedMemoryStream(buildInfo.PdbMetadataReader.MetadataPointer, buildInfo.PdbMetadataReader.MetadataLength), |
There was a problem hiding this comment.
new UnmanagedMemoryStream [](start = 31, length = 25)
This wasn't introduced by your change but ... should we Dispose() this instance after calling ToXml()?
There was a problem hiding this comment.
It's like MemoryStream in that it has a no-op dispose and no finalizer so it's not necessary. When easy I toss it into a using but that's more pedantic behavior than anything else.
This updates our Visual Basic build provenance work such that I can now successfully rebuild a "Hello World" style application.
Want to get this merged because it's an incremental step forward. Then once it's merged and some of @RikkiGibson work is merged I will go back to getting Microsoft.CodeAnalysis.VisualBasic into the verified rebuild list.