-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Support build/restore of file-based programs #48662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for building and restoring file‐based programs by introducing new virtual commands and updating argument names across various commands. Key changes include:
- Replacing the old SlnOrProjectArgument with SlnOrProjectOrFileArgument.
- Introducing VirtualProjectBuildingCommand and related virtual command types for file-based program processing.
- Updating documentation to reflect support for file-based commands.
Reviewed Changes
Copilot reviewed 24 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Workload/Restore/WorkloadRestoreCommandParser.cs | Updated argument usage for workload restore. |
| src/Cli/dotnet/Commands/Workload/Restore/WorkloadRestoreCommand.cs | Adjusted argument references for restore commands. |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Introduced virtual command logic for file-based runs. |
| src/Cli/dotnet/Commands/Run/RunCommand.cs | Refactored virtual command creation. |
| src/Cli/dotnet/Commands/Restore/RestoringCommand.cs | Updated restore command type to ForwardingRestoreCommand. |
| src/Cli/dotnet/Commands/Restore/RestoreCommandParser.cs | Updated argument naming and restore command creation. |
| src/Cli/dotnet/Commands/Restore/RestoreCommand.cs | Converted RestoreCommand to an abstract class with concrete implementations. |
| src/Cli/dotnet/Commands/Build/BuildCommandParser.cs & BuildCommand.cs | Updated argument names and introduced virtual and forwarding build commands. |
| documentation/general/dotnet-run-file.md | Updated documentation to include file-based command support. |
Files not reviewed (10)
- src/Cli/dotnet/CliStrings.resx: Language not supported
- src/Cli/dotnet/xlf/CliStrings.cs.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.de.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.es.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.fr.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.it.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.ja.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.ko.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.pl.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.pt-BR.xlf: Language not supported
jasonmalinowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing off that I've reviewed it, but not sure my signoff means much. I'm can't speak if any of the introduction of virtual command objects is the right pattern to do or not.
| string[] forwardedOptions = result.OptionValuesToBeForwarded(RestoreCommandParser.GetCommand()).ToArray(); | ||
|
|
||
| msbuildArgs.AddRange(result.GetValue(RestoreCommandParser.SlnOrProjectArgument) ?? []); | ||
| if (fileArgument is [{ } arg] && VirtualProjectBuildingCommand.IsValidEntryPointPath(arg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (fileArgument is [{ } arg] && VirtualProjectBuildingCommand.IsValidEntryPointPath(arg)) | |
| if (fileArgument is [{ } arg] && VirtualProjectCommandHelpers.IsValidEntryPointPath(arg)) |
Or move that to some other similarly-named type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning to rename that to something like FileBasedProgramHelpers to match the API that will be extracted out of it into Roslyn (dotnet/roslyn#78159). I'd rather do that in a separate PR to avoid large diffs (if Git wouldn't recognize it as a rename)
| var restoreRequest = new BuildRequestData( | ||
| CreateProjectInstance(projectCollection, addGlobalProperties: static (globalProperties) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new in this PR, but if there's now a restore command directly, is there some way to share this logic that's setting the environment? Otherwise it seems we risk a change to dotnet restore RealProject.csproj not being updated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's currently not possible. The real project-based restore calls msbuild.exe where this logic lives internally. Here we need to call msbuild API where the logic is not exposed. There is a tracking issue linked in the comment above to publish an API for that.
|
@RikkiGibson @chsienki @MiYanni for reviews, thanks |
|
|
||
| public sealed class ForwardingRestoreCommand : RestoreCommand | ||
| { | ||
| public ForwardingRestoreCommand(IEnumerable<string> msbuildArgs, string msbuildPath = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect I'm missing something, but I don't fully understand why we need to wrap the MSBuildForwardingApp in a command. I assume the type hierarchy requires it, but it seems like it should be possible to just have an 'executable command' or something that implements Execute and then we wouldn't need as many type wrappers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, with some refactoring we can do that, let me try it. Thanks.
MiYanni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a fine way to do this. There's likely a cleaner way, but would require more code changes, and I have no concrete suggestion currently. Just something for me to keep in mind going forward.
|
I'm using P7. I ran |
|
In preview 7 I get the following behavior Do you have any env vars/profile settings that are customizing the msbuild logging in any way? |
rich@richs-MacBook-Pro test-file-based-apps % cat app.cs
Console.WriteLine("Hello World!");%
rich@richs-MacBook-Pro test-file-based-apps % dotnet build app.cs
rich@richs-MacBook-Pro test-file-based-apps % ls
app.cs
rich@richs-MacBook-Pro test-file-based-apps % dotnet --version
10.0.100-preview.7.25380.108
rich@richs-MacBook-Pro test-file-based-apps % export | grep DOTNET
rich@richs-MacBook-Pro test-file-based-apps % uname -a
Darwin richs-MacBook-Pro.local 25.0.0 Darwin Kernel Version 25.0.0: Fri Jul 25 23:19:02 PDT 2025; root:xnu-12377.0.187.0.2~21/RELEASE_ARM64_T6041 arm64I see the As you can likely guess from |
|
I'm more than happy to file a separate issue. No need to re-use this PR. Just seemed like the place to start. |
|
If the issue also occurs for ordinary projects I would file an issue on msbuild as it's likely their logger misbehaving, if it only occurs for file-based apps then would file it in this repo. |
|
Specific to FBA. Will file an issue here. |
Resolves #48551.