[browser] Add chrome provisioning on macos#124852
[browser] Add chrome provisioning on macos#124852radekdoulik wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
This pull request adds Chrome browser and ChromeDriver provisioning support for macOS to enable WASM browser testing on macOS. The changes align with existing Linux and Windows provisioning configurations.
Changes:
- Added macOS-specific Chrome version properties to BrowserVersions.props (version, revision, snapshot URL, and V8 version)
- Added macOS PropertyGroup in wasm-provisioning.targets to configure Chrome, ChromeDriver, and V8 binary paths and download URLs
- Updated error messages in three provisioning targets to include macOS as a supported platform
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| eng/testing/BrowserVersions.props | Adds macOS Chrome/V8 version properties matching Linux/Windows versions (143.0.7499.40, revision 1536371, V8 14.3.127) |
| eng/testing/wasm-provisioning.targets | Adds macOS configuration block with directory names, binary names, URLs, and updates error conditions to include macOS support |
| <_ChromeOSPrefix>Mac_Arm</_ChromeOSPrefix> | ||
|
|
||
| <ChromeVersion>$(macos_ChromeVersion)</ChromeVersion> | ||
| <ChromeRevision>$(macos_ChromeRevision)</ChromeRevision> | ||
| <_ChromeBaseSnapshotUrl>$(macos_ChromeBaseSnapshotUrl)</_ChromeBaseSnapshotUrl> | ||
|
|
||
| <ChromeUrl>$(macos_ChromeBaseSnapshotUrl)/chrome-mac.zip</ChromeUrl> | ||
| <ChromeDriverUrl>$(macos_ChromeBaseSnapshotUrl)/chromedriver_mac64.zip</ChromeDriverUrl> | ||
|
|
||
| <V8Version>$(macos_V8Version)</V8Version> | ||
| <V8DirName>v8-$(macos_V8Version)</V8DirName> | ||
| <V8BinaryName>$(V8DirName).sh</V8BinaryName> | ||
| <_V8PlatformId>mac-arm64</_V8PlatformId> |
There was a problem hiding this comment.
The configuration hardcodes Mac_Arm for the ChromeOSPrefix and mac-arm64 for V8PlatformId, which only supports ARM-based Macs. Intel-based Macs (x64) would not be supported with this configuration. Consider detecting the architecture dynamically using RuntimeInformation.ProcessArchitecture or providing separate configurations for both Mac_Arm and Mac x86_64, similar to how Windows uses Win_x64 and Linux uses Linux_x64. The chromium-browser-snapshots bucket typically has both Mac_Arm and Mac folders for different architectures.
There was a problem hiding this comment.
I think we don't need to add intel mac support to provisioning now. It would be good to update the checks to report error on intel macs.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
|
||
| <Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows'))" | ||
| Text="Chrome provisioning only supported on Linux, and windows." /> | ||
| <Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows')) and !$([MSBuild]::IsOSPlatform('macos'))" |
There was a problem hiding this comment.
The Error condition uses IsOSPlatform('macos'), but MSBuild recognizes 'OSX' as the platform identifier for macOS, not 'macos'. This should be changed to IsOSPlatform('OSX') to be consistent with the rest of the codebase (see eng/OSArch.props line 4).
| <Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows')) and !$([MSBuild]::IsOSPlatform('macos'))" | |
| <Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows')) and !$([MSBuild]::IsOSPlatform('OSX'))" |
| <Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows')) and !$([MSBuild]::IsOSPlatform('macos'))" | ||
| Text="V8 provisioning only supported on Linux, Windows, and macOS." /> |
There was a problem hiding this comment.
The Error condition uses IsOSPlatform('macos'), but MSBuild recognizes 'OSX' as the platform identifier for macOS, not 'macos'. This should be changed to IsOSPlatform('OSX') to be consistent with the rest of the codebase (see eng/OSArch.props line 4).
| <PropertyGroup Condition="$([MSBuild]::IsOSPlatform('macos'))"> | ||
| <ChromeDirName>chrome-mac/Chromium.app/Contents/MacOS</ChromeDirName> | ||
| <ChromeDriverDirName>chromedriver_mac64</ChromeDriverDirName> | ||
| <ChromeBinaryName>Chromium</ChromeBinaryName> | ||
| <ChromeDriverBinaryName>chromedriver</ChromeDriverBinaryName> | ||
| <_ChromeOSPrefix>Mac_Arm</_ChromeOSPrefix> | ||
|
|
||
| <ChromeVersion>$(macos_ChromeVersion)</ChromeVersion> | ||
| <ChromeRevision>$(macos_ChromeRevision)</ChromeRevision> | ||
| <_ChromeBaseSnapshotUrl>$(macos_ChromeBaseSnapshotUrl)</_ChromeBaseSnapshotUrl> | ||
|
|
||
| <ChromeUrl>$(macos_ChromeBaseSnapshotUrl)/chrome-mac.zip</ChromeUrl> | ||
| <ChromeDriverUrl>$(macos_ChromeBaseSnapshotUrl)/chromedriver_mac64.zip</ChromeDriverUrl> | ||
|
|
||
| <V8Version>$(macos_V8Version)</V8Version> | ||
| <V8DirName>v8-$(macos_V8Version)</V8DirName> | ||
| <V8BinaryName>$(V8DirName).sh</V8BinaryName> | ||
| <_V8PlatformId>mac-arm64</_V8PlatformId> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
The ChromeOSIdentifier property is not being set for macOS. Lines 8-10 set it for Windows and Linux but not for macOS. This means on macOS it will default to "unsupported-platform", which could cause unexpected behavior. You should add a line like:
ChromeOSIdentifier Condition="$([MSBuild]::IsOSPlatform('macos'))">macOS
between lines 9 and 10 in eng/testing/wasm-provisioning.targets.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
No description provided.