Support multiple files in file-based programs #48782
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the .NET CLI to support multiple file input in file-based programs. Key changes include updates to test expectations in RunFileTests, reordering of conditional checks in RunCommand, and a major refactoring of the project conversion command to handle both files and directories (including supporting a shared directory).
Reviewed Changes
Copilot reviewed 11 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Updated tests to reflect new behavior for error output and file handling with multiple entry points. |
| test/Microsoft.NET.TestFramework/Assertions/DirectoryInfoAssertions.cs | Added a new helper method to validate directory subtrees. |
| src/Cli/dotnet/Commands/Run/RunCommand.cs | Reordered conditionals to throw if NoCache is specified before processing the entry point file. |
| src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommandParser.cs | Renamed and updated arguments to support both files and directories and added a shared directory option. |
| src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs | Refactored the conversion logic to support multiple entry points, shared directories, and file/directory conversion. |
| src/Cli/Microsoft.DotNet.Cli.Utils/PathUtility.cs | Introduced SafeRenameDirectory to handle directory renaming with safeguards against conflicts. |
Files not reviewed (14)
- src/Cli/dotnet/Commands/CliCommandStrings.resx: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.cs.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.de.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.es.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.fr.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.it.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.ja.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.ko.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.pl.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.pt-BR.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.ru.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.tr.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hans.xlf: Language not supported
- src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hant.xlf: Language not supported
Comments suppressed due to low confidence (1)
src/Cli/Microsoft.DotNet.Cli.Utils/PathUtility.cs:428
- [nitpick] Validate that the temporary directory fallback mechanism in SafeRenameDirectory does not introduce security risks in environments with restricted directory permissions.
if (IsChildOfDirectory(sourceDir, destDir))
|
@dotnet/run-file for reviews, thanks |
| @@ -416,4 +416,60 @@ public static string GetDirectorySeparatorChar() | |||
|
|
|||
| return null; | |||
| } | |||
|
|
|||
| public static void SafeRenameDirectory(string sourceDir, string destDir) | |||
There was a problem hiding this comment.
I haven't had time yet to review the implementation in detail, or to see how this is used, but, this looks like something difficult to get right. File systems can do weird stuff--hard links, mount points, etc.
Is this really the kind of thing where we want to automatically figure out what to do, rather than just complaining that the file system isn't in a state where we can do it automatically, and user needs to make the appropriate change to allow the tool to work?
There was a problem hiding this comment.
I'm not opposed to failing more instead of trying to be smart. In this case, I see these options:
- Simply call
Directory.Movewithout any checks. That will fail with an exception in case we are trying to move a directory into itself. And you are left in half-migrated state. We don't want that. - Do some checks up front and fail early. So we still need some logic to detect that
Directory.Movewon't work. Since we need to detect this situation anyway, why not do the automatic thing (moving to temp directory as a middle step)? I'm not sure, just curious what you think. - Have this
SafeRenameDirectoryutility. It seems it might be useful in other cases for the CLI - basically if you have a directory like/a/b, you should be able to "rename" it to/a/b/cas programmer and this utility allows that. For example, vscode file explorer can do this as well.
| // Check entry-point file path. | ||
| string fileOrDirectory = Path.GetFullPath(_fileOrDirectory); | ||
| bool isFile = VirtualProjectBuildingCommand.IsValidEntryPointPath(fileOrDirectory); | ||
| if (!isFile && (File.Exists(fileOrDirectory) || !Directory.Exists(fileOrDirectory))) |
There was a problem hiding this comment.
I didn't fully understand the File.Exists(fileOrDirectory) || !Directory.Exists(fileOrDirectory) check. Is there a case that would behave incorrectly if this were replaced with !Directory.Exists(fileOrDirectory)?
There was a problem hiding this comment.
No, will simplify, thanks.
| { | ||
| try | ||
| { | ||
| using var stream = File.Open(cacheFile.FullName, FileMode.Open, FileAccess.Read, FileShare.Read); |
Corresponding spec update: #48437 (already merged and available at https://github.com/dotnet/sdk/blob/main/documentation/general/dotnet-run-file.md)
Resolves #48174.