File-based apps: add support for #:include#52347
File-based apps: add support for #:include#52347jjonescz wants to merge 18 commits intodotnet:release/10.0.3xxfrom
#:include#52347Conversation
bf9dc98 to
d13a3f2
Compare
d13a3f2 to
c06c1fc
Compare
c06c1fc to
d64bb1a
Compare
| VirtualProjectBuilder.WriteProjectFile( | ||
| csprojWriter, | ||
| directives, | ||
| evaluatedDirectives, |
There was a problem hiding this comment.
I've noticed the run-api doesn't evaluate directives, so I've fixed that, and added tests for it.
| <value>Unrecognized directive '{0}'.</value> | ||
| <comment>{0} is the directive name like 'package' or 'sdk'.</comment> | ||
| </data> | ||
| <data name="EmptyTempPath" xml:space="preserve"> |
There was a problem hiding this comment.
This is not used in the package, so moved out.
|
|
||
| public readonly struct ParseInfo | ||
| { | ||
| public required SourceFile SourceFile { get; init; } |
There was a problem hiding this comment.
Need to know source file that directives come from to evaluate relative paths in #:include/#:exclude directives, so moved this from ParseContext to ParseInfo.
| } | ||
|
|
||
| internal delegate void ErrorReporter(SourceFile sourceFile, TextSpan textSpan, string message); | ||
| internal delegate void ErrorReporter(SourceFile sourceFile, TextSpan textSpan, string message, Exception? innerException = null); |
There was a problem hiding this comment.
The inner exception used to be passed but got lost in this refactoring: #51995, so I've restored it.
| } | ||
|
|
||
| public static void RemoveDirectivesFromFile(ImmutableArray<CSharpDirective> directives, SourceText text, string filePath) | ||
| public static void RemoveDirectivesFromFile(ImmutableArray<CSharpDirective> directives, SourceFile sourceFile, string targetFilePath) |
There was a problem hiding this comment.
This function did not used to save the file if not modified - I don't think that was intentional but it worked thanks to the project convert command copying all Compile items anyway. It all works differently (better) now.
|
|
||
| private static void Convert(string inputCSharp, out string actualProject, out string? actualCSharp, bool force, string? filePath, | ||
| bool collectDiagnostics, out ImmutableArray<SimpleDiagnostic>.Builder? actualDiagnostics) | ||
| private static void Convert( |
There was a problem hiding this comment.
Unified the helpers here.
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
|
I don't know why this should be closed, reopening. |
| Relative paths are resolved relative to the file containing the directive. | ||
|
|
||
| This is currently gated under a feature flag that can be enabled by setting the MSBuild property `ExperimentalFileBasedProgramEnableIncludeDirective=true`. | ||
| This directive is currently gated under a feature flag that can be enabled by setting the MSBuild property `ExperimentalFileBasedProgramEnableIncludeDirective=true`. |
There was a problem hiding this comment.
Suitably verbose property name ![]()
|
It got closed unintentionally because the base branch (3xx) got re-created. |
|
@333fred @RikkiGibson @MiYanni this is ready for reviews, thanks :) |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM, had various comments/questions, which you can feel free to address in this PR or a future PR (moving to some tracking issue).
test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds support for multi-file file-based apps by introducing #:include / #:exclude directives (with transitive directive evaluation), and improves dotnet run’s file-based caching/up-to-date behavior to account for additional non-entrypoint inputs (e.g., Razor/default items), with corresponding test and doc updates.
Changes:
- Implement
#:include/#:excludedirective parsing, evaluation, and item-type mapping; evaluate directives transitively across includedCompileitems. - Update file-based
dotnet runcaching/up-to-date logic to track additional inputs and improve binlog evaluation coverage. - Update
dotnet project convertbehavior and tests to copy/convert included files, plus new test assertions and documentation updates.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Adds extensive coverage for include/exclude directives, transitive directives, binlog evaluation behavior, and up-to-date/default item scenarios (incl. Razor). |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Updates conversion test harness to optionally evaluate directives via MSBuild and adds include/exclude conversion tests. |
| test/Microsoft.NET.TestFramework/Assertions/DirectoryInfoAssertions.cs | Adds helper assertions for file content/pattern and subtree verification to support new conversion tests. |
| src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs | Core implementation: directive evaluation (incl. include/exclude), transitive directive collection over included compile items, mapping support, and safer directive removal. |
| src/Microsoft.DotNet.ProjectTools/Resources.resx | Adds new localized resource strings used by file-based project tooling. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.zh-Hant.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.zh-Hans.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.tr.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.ru.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.pt-BR.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.pl.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.ko.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.ja.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.it.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.fr.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.es.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.de.xlf | Adds new localization entries for new resource strings. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.cs.xlf | Adds new localization entries for new resource strings. |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Extends caching/up-to-date logic with “additional sources” tracking; improves restore session ID consistency; adjusts binlog shutdown behavior. |
| src/Cli/dotnet/Commands/Run/Api/RunApiCommand.cs | Uses VirtualProjectBuilder MSBuild-backed evaluation to produce a project that reflects evaluated directives. |
| src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs | Updates conversion to copy/convert included compile files and uses mapping to decide which items to copy. |
| src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs | Adds verbose diagnostic when a package isn’t found in assets and minor LINQ cleanup. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/InternalAPI.Unshipped.txt | Records new internal API surface for include/exclude directives and related helpers. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs | Adds parsing model changes (source file in parse info), include/exclude directive support, mapping parsing, and richer error reporting. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/FileBasedProgramsResources.resx | Adds new resource strings for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hant.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hans.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.tr.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.ru.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.pt-BR.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.pl.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.ko.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.ja.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.it.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.fr.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.es.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.de.xlf | Adds new localization entries for include/exclude and mapping errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.cs.xlf | Adds new localization entries for include/exclude and mapping errors. |
| documentation/general/dotnet-run-file.md | Updates file-based app behavior docs to describe multi-file support, include/exclude directives, mappings, and caching implications. |
Comments suppressed due to low confidence (1)
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:265
- The MSBuild-required environment variables are being set twice (two identical foreach loops). This also breaks restoration because the second loop overwrites the saved original values with the already-modified values, so the finally block won’t restore the real originals. Remove the duplicate loop (keep a single set/restore pass).
// Set environment variables for MSBuild.
foreach (var (key, value) in MSBuildForwardingAppWithoutLogging.GetMSBuildRequiredEnvironmentVariables())
{
savedEnvironmentVariables[key] = Environment.GetEnvironmentVariable(key);
Environment.SetEnvironmentVariable(key, value);
}
// Set environment variables.
foreach (var (key, value) in MSBuildForwardingAppWithoutLogging.GetMSBuildRequiredEnvironmentVariables())
{
savedEnvironmentVariables[key] = Environment.GetEnvironmentVariable(key);
Environment.SetEnvironmentVariable(key, value);
}
|
@333fred for another review, thanks |
333fred
left a comment
There was a problem hiding this comment.
Done reviewing the implementation. I have not looked at the tests.
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
| if (Name.EndsWith(entry.Extension, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| itemType = entry.ItemType; | ||
| break; |
There was a problem hiding this comment.
Should we detect and error on duplicates?
There was a problem hiding this comment.
Hm. Do you mean report an error when the mapping has duplicate keys like .cs=X;.cs=Y? What if someone wants to extend the mapping via <Mapping>.cs=Z;$(Mapping)</Mapping>, it would be difficult to ensure there are no duplicates.
There was a problem hiding this comment.
Is that valid today? And is it earliest wins? If so, then I'm ok with it.
There was a problem hiding this comment.
If by "today" you mean in this PR (as this feature didn't exist before), then yes.
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
| string itemRelativePath = Path.GetRelativePath(relativeTo: entryPointFileDirectory, path: itemFullPath); | ||
| yield return (FullPath: itemFullPath, RelativePath: itemRelativePath); | ||
|
|
||
| // Files outside the source directory should be copied into the target directory at the top level. |
There was a problem hiding this comment.
This feels non-obvious and incorrect. If you're attempting to convert a project, we should preserve the same paths. If the user is trying to convert something that referenced a file with directives, well that's on them to fix. We shouldn't try to handle it or copy things around for them.
There was a problem hiding this comment.
I'm not sure what you mean, this handles even as simple scenarios as
// app.cs
#:include ../file.cs
#:include ../../file.cs
// ...Are you suggesting such file-based app should report an error during conversion?
I think our goal with dotnet project convert is to seamlessly convert as much file-based apps as possible.
There was a problem hiding this comment.
I don't know why that would have an issue. I'd expect the converted project to have
<Compile Include="..\file.cs" />
<Compile Include="..\..\file.cs" />I do not expect dotnet project convert to duplicate my source files, and I do not expect it to try and remove transitive references from such files (I still think transitive references are a bad idea in general, tbh).
There was a problem hiding this comment.
Hm, the general idea is that there are no <Compile Include /> items in the converted project. It should be as simple as if you ran dotnet new console - i.e., .cs/.razor/.resx files should be included implicitly from the project's directory.
Also, we need to remove file-level directives from #:included .cs files or the converted project won't work. Otherwise, you just end up with half-converted project, not sure what that would be useful for. (In that case it might be better to report errors, but if we can convert such file-based apps by copying the sources, why wouldn't we allow the user to do that?)
I do not expect
dotnet project convertto duplicate my source files
That's pre-existing behavior and very much by-design. There is even an open PR to add a choice for move vs. copy the source files: #52802
| { | ||
| // We intentionally ignore new additional sources in up-to-date check | ||
| // to avoid the overhead of MSBuild evaluation every time (even if the app is up to date). | ||
| // That can lead to missed changes but we are fine with that in rare cases |
There was a problem hiding this comment.
I'm not sure I understand the scenario, and definitely don't agree with being fine with up-to-date saying that rebuild is not necessary when it is.
There was a problem hiding this comment.
I'm not sure I understand the scenario
The up-to-date check detects changes to existing files but not addition of new files.
For example:
- Create a file-based app
app.csthat's just
#:property ExperimentalFileBasedProgramEnableIncludeDirective=true
#:include *.cs
Console.WriteLine();- Run
dotnet run app.cs- builds and runs the app. - Run
dotnet run app.cs- skips build, only runs the exe. - Add
.csfile somewhere in the directory cone ofapp.cs. - Run
dotnet run app.cs- doesn't check for all**/*.csfiles (could be slow I imagine if you have a script in the root of a large repo), instead it skips build again, and hence the added.csfile is not included in the compilation.
You can of course do dotnet build app.cs or dotnet run --no-cache app.cs to force a re-build.
See also test UpToDate_DefaultItems which tests this for .resx and IncludeDirective_UpToDate which I will extend to test this for .cs.
and definitely don't agree with being fine with up-to-date saying that rebuild is not necessary when it is.
That's a preexisting behavior as the comment says in "(another example: we ignore changes to implicit build files imported transitively)" and I believe we discussed it's fine for file-based apps that the up-to-date check is not perfect in some edge cases.
Anyway we can lift this limitation but it will need some work to do globbing on every dotnet run as part of the up-to-date check and try to make it fast as not to hinder that up-to-date optimization too much. Seems fine to me as a follow up especially considering this is an experimental opt-in feature currently. What do you think?
Filed #53068 which I will also reference from code.
There was a problem hiding this comment.
To my mind, as soon as you start #includeing with globbing, all expectations of sub-second no-msbuild for startup go out the window. I'm in heavy disagreement that it is acceptable for adding a .cs file in the cone when I've globbed should ever be missed; that behavior is incredibly non-obvious and outside the norm of expectation for .NET projects.
There was a problem hiding this comment.
Makes sense, thanks for pushing back, I agree and will look into this.
|
@333fred for another look, thanks |
| if (Directives.Any(static d => d is CSharpDirective.Project)) | ||
| { | ||
| Reporter.Verbose.WriteLine("Skipping computing cache because there are project directives."); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Can we reuse CanSaveCache here?
There was a problem hiding this comment.
No, this is a subset of CanSaveCache. This is a faster check that we can do before we have access to the virtual project.
|
|
||
| public const string MappingPropertyName = "FileBasedProgramsItemMapping"; | ||
|
|
||
| public static string DefaultMappingString => ".cs=Compile;.resx=EmbeddedResource;.json=None;.razor=Content"; |
There was a problem hiding this comment.
What needs this public API other than a test? Can it be removed?
There was a problem hiding this comment.
VirtualProjectBuilder uses it to put this into the virtual project file:
<FileBasedProgramsItemMapping>{CSharpDirective.IncludeOrExclude.DefaultMappingString}</FileBasedProgramsItemMapping>The API is internal btw since it lives inside internal class CSharpDirective.
There was a problem hiding this comment.
We could construct this DefaultMappingString from the DefaultMapping array inside VirtualProjectBuilder I guess if that's what you are suggesting.
Resolves #48174.
Fixes part of #50912 (changes to files are detected, new file additions are not, except for
#:include).