Automatically generate embedded file contents for optimized run-file path - #53086
#53086Automatically generate embedded file contents for optimized run-file path#53086
Conversation
There was a problem hiding this comment.
Thank you so much for taking action on this and keeping our repo maintainable.
I have seen several times before that some unintended changes tried to slip through here
I agree that the test is a good idea still. I wonder if we could also keep the hardcoded one and do something else besides failing the build - e.g. sending a notification to your team? Or having a workflow that triggers when the file changes and then it pings.
Anything that blocks the repo builds like this would be fine in a smaller or isolated repo with just roslyn stuff but it in the SDK if we let every team do this pattern the repo would break basically every day. I commend the error message your test had because it was readable and easy to know what to do.
|
I'm not completely against just removing the generated files from Git and generating them during build instead if this is causing unreasonable maintenance pain. I'm not sure how would we even implement "not fail test, ping team instead" or something similar while not spending too much time on such infra for this single test (maybe we could use the existing Build Analysis infrastructure?), and furthermore it would allow SDK to be in an inconsistent state if we allowed merge when the test fails, that doesn't sound very good. |
There was a problem hiding this comment.
Pull request overview
This PR implements automatic generation of embedded file content templates for the optimized dotnet run path, addressing issue #53074. Previously, the editorconfig template and other auxiliary file contents in CSharpCompilerCommand.cs were hardcoded, requiring manual updates when MSBuild added new CompilerVisibleProperty values. This change extends the existing CscArguments test to capture and auto-generate all file content methods.
Changes:
- Extended
CscArgumentstest to generate file content methods (editorconfig, assembly info, global usings, runtime config, assembly attributes) from MSBuild-emitted files - Replaced hardcoded file content templates in
CSharpCompilerCommand.cswith calls to generated methods - Added internal properties (
BaseDirectoryWithTrailingSeparator,FileNameWithoutExtension,TargetFramework) to support template generation - Updated error message to reference the
CscArgumentstest for regeneration
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Extends CscArguments test to generate file content methods by discovering emitted files, applying string replacements, and generating interpolated string methods. Updates error message to reference test for regeneration. |
| src/Cli/dotnet/Commands/Run/CSharpCompilerCommand.cs | Removes hardcoded file content templates, adds internal properties for template generation, replaces inline templates with calls to generated methods |
| src/Cli/dotnet/Commands/Run/CSharpCompilerCommand.Generated.cs | Adds five generated file content methods (RuntimeConfig, AssemblyAttributes, AssemblyInfo, GeneratedMSBuildEditorConfig, GlobalUsings) with proper string interpolation |
|
@RikkiGibson for another review, thanks |
| var generatedMethodName = GetGeneratedMethodName(emittedFileName); | ||
| if (generatedMethodName is null) | ||
| { | ||
| Log.WriteLine($"Skipping unrecognized file '{emittedFile}'."); |
There was a problem hiding this comment.
What happens if a new/unknown file gets added to the "default project build", where the file is an input to the compiler? Is there a risk that we will skip the file, not notice, and get wrong semantics in the optimized csc build case? It feels like we might want to fail this test in that case.
There was a problem hiding this comment.
The test below (CscVsMSBuild) should catch that.
Resolves #53074.