Ensure generated trees have a full path #60095
Conversation
- Use GeneratedFilesOutputDirectory as the base if provided - Fallback to OutputDirectory when not - Add tests and fix full PDB verification
|
@dotnet/roslyn-compiler for review please |
|
|
||
| Assert.Equal(expectedEmbeddedMap[docPath], sourceStr); | ||
| Assert.True(expectedEmbeddedMap.Remove(docPath)); | ||
| } | ||
| } | ||
| catch | ||
| finally |
There was a problem hiding this comment.
Turns out we were silently ignoring failures, but this function was not actually being used anyway :/
| @@ -14324,6 +14325,123 @@ public void TestDuplicateAdditionalFiles(string additionalFilePath1, string addi | |||
| [InlineData("abc/a.txt", "./../ABC/a.txt", 2)] | |||
| public void TestDuplicateAdditionalFiles_Linux(string additionalFilePath1, string additionalFilePath2, int expectedCount) => TestDuplicateAdditionalFiles(additionalFilePath1, additionalFilePath2, expectedCount); | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
How does this behavior interact with [CallerFilePath] optional arguments? Now that the source file has a full path would expect some interaction with this feature.
| driver = driver.RunGenerators(input); | ||
| var results = driver.GetRunResult(); | ||
|
|
||
| var treeRoot = this.Arguments.GeneratedFilesOutputDirectory ?? Arguments.OutputDirectory; |
There was a problem hiding this comment.
This makes me nervous because toggling $(EmitCompilerGeneratedFiles) will end up impacting which directory we use here. That ends up impacting determinism because it's possible /pathmap accounts for these directories differently.
Not sure how do this better though. Only thought I had is should we only give them full paths if GeneratedFilesOutputDirectory is set? Basically if the user wants full paths they have to help us out here.
There was a problem hiding this comment.
Yeah, this is the crux of the issue really, but @tmat was fairly explicit that they should always have full paths as that's whats causing downstream tooling breaks.
We do have a test in this PR that validates /pathmap is working as expected for GeneratedFilesOutputDirectory, so we should be good on that front. If the user turns on $(EmitCompilerGeneratedFiles) then yes the user will have to also path map the directory we use there (which is user controllable too)
There was a problem hiding this comment.
Seems like a logical requirement for /pathmap that it needs to cover the directory specified in GeneratedFilesOutputDirectory. In practice, it will likely be some subdirectory of the project directory.
| var results = driver.GetRunResult(); | ||
|
|
||
| var treeRoot = this.Arguments.GeneratedFilesOutputDirectory ?? Arguments.OutputDirectory; | ||
| var compilationOut = input.AddSyntaxTrees(results.GeneratedTrees.Select(t => t.WithFilePath(Path.Combine(treeRoot, t.FilePath)))); |
There was a problem hiding this comment.
No the results struct already contains them (see https://sourceroslyn.io/#Microsoft.CodeAnalysis/SourceGeneration/GeneratorDriver.cs,148)
| if (Directory.Exists(Arguments.GeneratedFilesOutputDirectory)) | ||
| { | ||
| Directory.CreateDirectory(Path.GetDirectoryName(path)!); | ||
| Directory.CreateDirectory(Path.GetDirectoryName(tree.FilePath)!); |
| // validate that *no* sources were written | ||
| Assert.Empty(Directory.GetDirectories(generatedDir.Path)); | ||
|
|
||
| // but we still have full paths (using output dir) in the PDBs |
There was a problem hiding this comment.
Should we verify the files were written to the expected directory?
ValidateWrittenSources(new() { { generatedDir.Path, new() { { "generatedSource.cs", generatedSource } } } });
``` #Closed
There was a problem hiding this comment.
So we're not actually emitting any files here, which we check that on line 14384.
| // validate that *no* sources were written | ||
| Assert.Empty(Directory.GetDirectories(generatedDir.Path)); | ||
|
|
||
| // but we still have full paths (using output dir) in the PDBs |
There was a problem hiding this comment.
Should we verify the files were written to the expected directory?
ValidateWrittenSources(new() { { Path.Combine(mappedPath, generatorPrefix), new() { { "generatedSource.cs", generatedSource } } } });
``` #Closed
| @@ -2451,13 +2451,14 @@ private static void ValidateEmbeddedSources_Windows(Dictionary<string, string> e | |||
| continue; | |||
| } | |||
|
|
|||
| var sourceStr = Encoding.UTF8.GetString(sourceBlob.Array, sourceBlob.Offset, sourceBlob.Count); | |||
| // offset by 3 to skip the BOM | |||
There was a problem hiding this comment.
Odd to do this unconditionally. What if there is no BOM?
|
Issue #51998 also mentions potential problem with |
|
@chsienki this PR is essentially one year stale now. If we're not actively working on it can we move it to draft? |
Fixes #51998
Fixes #51773