Include some default items in file-based apps#49584
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR resolves #49557 by including default items in file‑based apps and adjusting the related test and project conversion logic. Key changes include:
- Updating tests in RunFileTests and DotnetProjectConvertTests to verify new default item behaviors.
- Replacing the PrepareProjectInstance method with a lazy-initialized Directives property in VirtualProjectBuildingCommand.
- Modifying XML project properties to use separate flags for default compile and embedded resource items.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Adds tests covering override scenarios for default compile and embedded resources. |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Introduces tests for file-based project conversion and default item inclusion/exclusion. |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Refactors directive initialization by removing PrepareProjectInstance and adding a lazy-initialized Directives property. |
| src/Cli/dotnet/Commands/Run/RunCommand.cs & src/Cli/dotnet/Commands/Run/Api/RunApiCommand.cs | Updates usage in Run commands to accommodate the removal of PrepareProjectInstance. |
| src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs | Enhances conversion logic to copy additional default items based on metadata. |
| documentation/general/dotnet-run-file.md | Updates documentation to reflect new virtual project properties and file conversion behavior. |
Comments suppressed due to low confidence (1)
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:136
- The backing field 'field' referenced in the Directives property's getter is not declared in the provided snippet. Please ensure a proper backing field is defined to support the lazy initialization of Directives.
if (field.IsDefault)
|
@RikkiGibson @MiYanni for reviews, thanks |
test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs
Show resolved
Hide resolved
| > [!CAUTION] | ||
| > Multi-file support is postponed for .NET 11. | ||
| > In .NET 10, only the single file passed as the command-line argument to `dotnet run` is part of the compilation. | ||
| > Specifically, the virtual project has properties `EnableDefaultCompileItems=false` and `EnableDefaultEmbeddedResourceItems=false` |
There was a problem hiding this comment.
How did we arrive at disabling this particular set of items? Do we have a reason for believing that if the directory contains some embedded resource, that we don't want to include it in the file-based app?
There was a problem hiding this comment.
Compile items are disabled since we are doing single file apps.
Embedded resources were discussed in the internal chat and I think the decision was to also exclude them, right, @baronfel @DamianEdwards? I'm not sure there is a strong reason for that. I think I would also prefer to include everything unless explicitly disabled (would make it easier to reason about this).
There was a problem hiding this comment.
Keeping embedded resources seems fine to me. @baronfel what was your reasoning for excluding them from single-file apps?
test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs
Show resolved
Hide resolved
|
Are there any tests which put |
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM with some more test suggestions, thanks
|
|
||
| // If '-' is specified as the input file, read all text from stdin into a temporary file and use that as the entry point. | ||
| entryPointFilePath = Path.GetTempFileName(); | ||
| // We create a new directory for each file so other files are not included in the compilation. |
There was a problem hiding this comment.
The CI failed, revealing a bug here, so I'm fixing it in the last commit.
Previously, the implementation for running code from stdin put the code into a temp file but with this PR other files in the temp directory could be potentially included in the compilation. I've fixed that by putting the stdin code files into isolated temp directories instead. I've reused the same machinery we have for creating the artifacts folders. I think both these kinds of temp dirs (for artifacts and for stdin code) can reuse other features when implemented (periodical cleanup and customizing the temp path).
There was a problem hiding this comment.
Is there a risk that Path.GetRandomFileName() will collide with another one from a previous run? Are we able to proceed and do something reasonable in that case?
I am curious if we could just think of each user having their own "stdin app" and reusing the artifacts path for it across runs. However, I don't think it's urgent/necessary (or even necessarily correct?) to do so.
There was a problem hiding this comment.
Is there a risk that
Path.GetRandomFileName()will collide with another one from a previous run? Are we able to proceed and do something reasonable in that case?
We should probably create the file only if it doesn't exist and fail if it does. Thanks.
If that happens often, we can improve on the random file name generation.
I am curious if we could just think of each user having their own "stdin app" and reusing the artifacts path for it across runs. However, I don't think it's urgent/necessary (or even necessarily correct?) to do so.
Hm, interesting, but that would disallow users running multiple stdin apps in parallel.
|
|
||
| new DirectoryInfo(testInstance.Path) | ||
| .EnumerateFileSystemInfos().Select(f => f.Name).Order() | ||
| .Should().BeEquivalentTo(["Directory.Build.targets", "Program", "Resources.resx", "Util.cs", "my.json", "second.json"]); |
There was a problem hiding this comment.
Did we make a decision that only the entry file should actually be moved, and other implicitly used files should be copied? I don't think I have any strong opposition to that, I just found it strange. It feels like it means user is going to have to cleanup in either case:
- They did want the file(s) moved. Go in and delete the originals, checking the conversion output and looking for the corresponding files in containing folder.
- They didn't want the file(s) moved. Delete from the conversion output, adjust/add project items to refer to originals.
It feels less likely they will want the file to remain in both locations
There was a problem hiding this comment.
I do feel satisfied with just shipping with this behavior for now, seeing what kind of feedback we get, and considering other ways of improving the convert experience in the future. Since that is not a frequently performed operation it feels like we should have space to refine how that works over time to some extent.
There was a problem hiding this comment.
Did we make a decision that only the entry file should actually be moved, and other implicitly used files should be copied?
My reasoning for this decision is that the implicit files might be used by other entry points that weren't converted. With #49624 we are going to ask users whether they want to keep the source files (i.e., the original entry point file would also be kept; or all included items would be moved).
I guess we could also ask users what to do about the included items separately (move/copy/nothing) but perhaps that's unnecessarily complicated - if users want to do something special, they can choose to keep the source files and clean up on their own, otherwise copying/moving all items sounds like a good set of default options.
5 participants
Resolves #49557.