-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[browser] rename WasmPerfTracing #113222
[browser] rename WasmPerfTracing #113222
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.
PR Overview
This PR renames the property and related references from "FeaturePerfTracing" to "WasmPerfTracing" to provide a more accurate naming convention in the browser-related code.
- Renamed property in BuildOptions, MSBuildOptions, PublishOptions, and related test and task files.
- Updated environment variable references in rollup and ES6 library files to match the new naming.
Reviewed Changes
File | Description |
---|---|
src/mono/wasm/Wasm.Build.Tests/BrowserStructures/BuildOptions.cs | Renamed property from FeaturePerfTracing to WasmPerfTracing |
src/mono/wasm/Wasm.Build.Tests/WasmSdkBasedProjectProvider.cs | Updated property reference for WasmPerfTracing |
src/mono/wasm/Wasm.Build.Tests/BrowserStructures/MSBuildOptions.cs | Renamed property to WasmPerfTracing |
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ComputeWasmBuildAssets.cs | Renamed property and updated parameter usage |
src/mono/wasm/Wasm.Build.Tests/DiagnosticsTests.cs | Updated constructor call to use WasmPerfTracing |
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ComputeWasmPublishAssets.cs | Renamed property and updated related method calls |
src/mono/browser/runtime/rollup.config.js | Updated environment variable check from FEATURE_PERFTRACING to WASM_PERFTRACING |
src/mono/browser/runtime/es6/dotnet.es6.lib.js | Updated environment variable check and usage for WasmPerfTracing |
src/mono/wasm/Wasm.Build.Tests/BrowserStructures/PublishOptions.cs | Renamed property from FeaturePerfTracing to WasmPerfTracing |
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/AssetsComputingHelper.cs | Renamed parameter from featurePerfTracing to wasmPerfTracing in candidate filtering |
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/mono/browser/runtime/es6/dotnet.es6.lib.js:93
- There is a duplicate key for 'enablePerfTracing' in the postset string; please review if the second occurrence should use a different key or be removed to avoid conflicting configuration.
`enablePerfTracing: ${ENABLE_BROWSER_PROFILER ? "true" : "false"}, ` +
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.
LGTM
/ba-g CI timeout |
@@ -87,7 +87,7 @@ function injectDependencies() { | |||
DotnetSupportLib["$DOTNET__postset"] = `DOTNET.setup({ ` + | |||
`wasmEnableSIMD: ${WASM_ENABLE_SIMD ? "true" : "false"},` + | |||
`wasmEnableEH: ${WASM_ENABLE_EH ? "true" : "false"},` + | |||
`enablePerfTracing: ${FEATURE_PERFTRACING ? "true" : "false"}, ` + | |||
`enablePerfTracing: ${WASM_PERFTRACING ? "true" : "false"}, ` + |
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.
is enablePerfTracing being set twice?
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.
ohh great catch 😄
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.
thank you, it should be set when ENABLE_BROWSER_PROFILER or WASM_PERFTRACING
I will address it in next EP PR
Do not trigger re-link for unit tests.
The default
FeaturePerfTracing
is false for the runtime pack.The unit tests set it to true in
src\tests\Directory.Build.targets
->clr.featuredefines.props
This PR will disconnect them.
Contributes to #113025