-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[browser][coreCLR] fix incremental build #122798
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
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 incremental build issues for browser/CoreCLR by addressing build trigger dependencies and file comparison problems on Windows. The changes ensure that JavaScript file modifications properly trigger CMake rebuilds and prevent unnecessary recompilation due to spurious file differences.
Key changes:
- Added JS file dependencies to browserhost CMakeLists to trigger rebuilds when JavaScript files change
- Fixed version file comparison in PowerShell script to prevent false positives from newline inconsistencies
- Removed obsolete browserhost sample code that was no longer needed
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/Common/JavaScript/CMakeLists.txt | Removed reference to obsolete sample file dotnet.boot.js from ROLLUP_TS_SOURCES |
| src/native/corehost/browserhost/CMakeLists.txt | Added set_source_files_properties to make browserhost sources depend on JS files, triggering rebuild when they change; removed sample subdirectory |
| eng/native/version/copy_version_files.ps1 | Changed version file reading to remove -Raw parameter to fix newline comparison issue |
| docs/workflow/building/coreclr/wasm.md | Removed documentation reference to obsolete sample dotnet.boot.js file |
| src/native/corehost/browserhost/sample/* | Removed all obsolete sample files (main.mjs, index.html, dotnet.boot.js, ReadMe.md, HelloWorld.csproj, HelloWorld.cs, CMakeLists.txt) |
Comments suppressed due to low confidence (1)
eng/native/version/copy_version_files.ps1:21
- Removing the
-Rawparameter changes how the file content is read. Without-Raw,Get-Contentreturns an array of lines, while with-Rawit returns a single string. This creates an array-to-string comparison issue on line 21 where$version_file_contents(now an array) is compared to$current_contents(also an array).
In PowerShell, when comparing arrays with -ne, the operator acts as a filter returning elements that are not equal to the comparison value, rather than returning a boolean. This means the condition will behave unexpectedly.
To fix this while maintaining the goal of consistent newline handling, consider using -join "n"to convert both arrays to strings for comparison, or use-Raw` for both reads with proper string normalization.
$current_contents = Get-Content -Path $version_file_destination
$is_placeholder_file = $current_contents -match "@\(#\)Version N/A @Commit:"
} else {
$is_placeholder_file = $true
}
if ($is_placeholder_file -and $version_file_contents -ne $current_contents) {
_version.cwhere there is no change on Windows.copy_version_files.ps1. One was with-Rawand the other without.