Ensure generated trees have a full path by chsienki · Pull Request #60095 · dotnet/roslyn

chsienki · 2022-03-10T23:05:28Z

Use GeneratedFilesOutputDirectory as the base if provided
Fallback to OutputDirectory when not
Add tests and fix full PDB verification

Fixes #51998
Fixes #51773

- Use GeneratedFilesOutputDirectory as the base if provided - Fallback to OutputDirectory when not - Add tests and fix full PDB verification

chsienki · 2022-03-10T23:05:52Z

@dotnet/roslyn-compiler for review please

chsienki · 2022-03-11T18:27:02Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

                }
            }
-            catch
+            finally


Turns out we were silently ignoring failures, but this function was not actually being used anyway :/

jaredpar · 2022-03-14T20:10:08Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

        [InlineData("abc/a.txt", "./../ABC/a.txt", 2)]
        public void TestDuplicateAdditionalFiles_Linux(string additionalFilePath1, string additionalFilePath2, int expectedCount) => TestDuplicateAdditionalFiles(additionalFilePath1, additionalFilePath2, expectedCount);

+


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.

jaredpar · 2022-03-14T20:17:28Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

+            driver = driver.RunGenerators(input);
+            var results = driver.GetRunResult();
+
+            var treeRoot = this.Arguments.GeneratedFilesOutputDirectory ?? Arguments.OutputDirectory;


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.

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)

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.

cston · 2022-03-14T20:36:58Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

+            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))));


AddSyntaxTrees

Do we need to update PostInitTrees as well? #Closed

No the results struct already contains them (see https://sourceroslyn.io/#Microsoft.CodeAnalysis/SourceGeneration/GeneratorDriver.cs,148)

cston · 2022-03-14T20:39:44Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

                                if (Directory.Exists(Arguments.GeneratedFilesOutputDirectory))
                                {
-                                    Directory.CreateDirectory(Path.GetDirectoryName(path)!);
+                                    Directory.CreateDirectory(Path.GetDirectoryName(tree.FilePath)!);


tree.FilePath

Minor: Consider extracting a path local - in part, to minimize the changes here. #Closed

cston · 2022-03-14T22:00:17Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+            // validate that *no* sources were written
+            Assert.Empty(Directory.GetDirectories(generatedDir.Path));
+
+            // but we still have full paths (using output dir) in the PDBs


Should we verify the files were written to the expected directory?

ValidateWrittenSources(new() { { generatedDir.Path, new() { { "generatedSource.cs", generatedSource } } } }); ``` #Closed

So we're not actually emitting any files here, which we check that on line 14384.

cston · 2022-03-14T22:02:08Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+            // validate that *no* sources were written
+            Assert.Empty(Directory.GetDirectories(generatedDir.Path));
+
+            // but we still have full paths (using output dir) in the PDBs


Should we verify the files were written to the expected directory?

ValidateWrittenSources(new() { { Path.Combine(mappedPath, generatorPrefix), new() { { "generatedSource.cs", generatedSource } } } }); ``` #Closed

tmat · 2023-02-06T19:44:18Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

                    }

-                    var sourceStr = Encoding.UTF8.GetString(sourceBlob.Array, sourceBlob.Offset, sourceBlob.Count);
+                    // offset by 3 to skip the BOM


Odd to do this unconditionally. What if there is no BOM?

tmat · 2023-02-06T19:47:03Z

Issue #51998 also mentions potential problem with #line directives using full paths in source-generated files. Seems like that still needs to be addressed, right?

jaredpar · 2023-03-30T17:40:49Z

@chsienki this PR is essentially one year stale now. If we're not actively working on it can we move it to draft?

Ensure generated trees have a full path in PDBs

4430ab0

- Use GeneratedFilesOutputDirectory as the base if provided - Fallback to OutputDirectory when not - Add tests and fix full PDB verification

chsienki added Area-Compilers Feature - Source Generators Source Generators labels Mar 10, 2022

chsienki requested a review from a team as a code owner March 10, 2022 23:05

chsienki added 2 commits March 10, 2022 15:48

Add path map test

87026ff

Add sarif tests

e6ac0bf

chsienki mentioned this pull request Mar 11, 2022

Have the full path in sarif error loggers #51932

Closed

chsienki commented Mar 11, 2022

View reviewed changes

jaredpar reviewed Mar 14, 2022

View reviewed changes

cston reviewed Mar 14, 2022

View reviewed changes

Extract path to local

7aa7174

runfoapp bot mentioned this pull request Mar 29, 2022

[Flaky integration tests] IDE version '17.2.32314.265' unexpectedly dismissed the light bulb. #60461

Closed

jcouv requested a review from cston February 3, 2023 19:18

cston approved these changes Feb 6, 2023

View reviewed changes

tmat reviewed Feb 6, 2023

View reviewed changes

chsienki marked this pull request as draft March 30, 2023 18:34

chsienki closed this Oct 2, 2024

		[InlineData("abc/a.txt", "./../ABC/a.txt", 2)]
		public void TestDuplicateAdditionalFiles_Linux(string additionalFilePath1, string additionalFilePath2, int expectedCount) => TestDuplicateAdditionalFiles(additionalFilePath1, additionalFilePath2, expectedCount);

Conversation

chsienki commented Mar 10, 2022

Uh oh!

chsienki commented Mar 10, 2022

Uh oh!

chsienki Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

jaredpar Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

jaredpar Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

chsienki Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

tmat Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

tmat commented Feb 6, 2023

Uh oh!

jaredpar commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tmat Feb 6, 2023 •

edited

Loading

cston Mar 14, 2022 •

edited

Loading

cston Mar 14, 2022 •

edited

Loading

cston Mar 14, 2022 •

edited

Loading

cston Mar 14, 2022 •

edited

Loading