-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix interaction of "csc only after msbuild" optimization and implicit build files #52133
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
Fix interaction of "csc only after msbuild" optimization and implicit build files #52133
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 fixes a bug in the "CSC only after MSBuild" optimization where implicit build files (like Directory.Build.props) were not always checked before deciding whether to use CSC-only builds. The fix ensures that implicit build file changes are detected even when the source file is also modified.
Key Changes:
- Refactored the CSC arguments reusability check into a dedicated helper function
GetReasonToNotReuseCscArguments() - Reordered the checks in
NeedsToBuild()to always verify implicit build files before making CSC-only build decisions - Added comprehensive test coverage for the interaction between the optimization and implicit build files
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Adds new test CscOnly_AfterMSBuild_DirectoryBuildProps that verifies implicit build file changes (Directory.Build.props) are detected regardless of whether the source file is also modified, using CombinatorialData to test all scenarios |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Refactors CSC argument reusability checks into GetReasonToNotReuseCscArguments() helper, reorders source file modification checks to ensure implicit build files are always checked, and simplifies GetBuildLevel() by removing duplicated validation logic |
The implementation is clean and correct. The fix addresses the issue described in the PR by ensuring implicit build files are checked before the optimization decision, and the refactoring improves code maintainability by extracting the validation logic into a well-named helper function.
|
@333fred @RikkiGibson for more reviews, thanks |
The added test would fail without this fix. The first commit is a minimal fix, the second commit is just improving code a bit, the third commit optimizes the check futher.