Skip to content

[browser] Add chrome provisioning on macos #124852

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.

Sign up for GitHub

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

Open
radekdoulik wants to merge 1 commit into
base: main
Choose a base branch
from
+30 −6
Open

[browser] Add chrome provisioning on macos #124852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions eng/testing/BrowserVersions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
<linux_ChromeRevision>1536371</linux_ChromeRevision>
<linux_ChromeBaseSnapshotUrl>https://storage.googleapis.com/chromium-browser-snapshots/Linux_x64/1536376</linux_ChromeBaseSnapshotUrl>
<linux_V8Version>14.3.127</linux_V8Version>
<macos_ChromeVersion>143.0.7499.40</macos_ChromeVersion>
<macos_ChromeRevision>1536371</macos_ChromeRevision>
<macos_ChromeBaseSnapshotUrl>https://storage.googleapis.com/chromium-browser-snapshots/Mac_Arm/1536376</macos_ChromeBaseSnapshotUrl>
<macos_V8Version>14.3.127</macos_V8Version>
<win_ChromeVersion>143.0.7499.40</win_ChromeVersion>
<win_ChromeRevision>1536371</win_ChromeRevision>
<win_ChromeBaseSnapshotUrl>https://storage.googleapis.com/chromium-browser-snapshots/Win_x64/1536376</win_ChromeBaseSnapshotUrl>
Expand Down
32 changes: 26 additions & 6 deletions eng/testing/wasm-provisioning.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,26 @@
<_V8PlatformId>linux64</_V8PlatformId>
</PropertyGroup>

<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>
Comment on lines +77 to +89
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@radekdoulik radekdoulik Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@radekdoulik radekdoulik Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Copy link
Member

@akoeplinger akoeplinger Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we should add support for windows/linux arm64 as well, and at that point I guess it's not hard to just add a case for x64 macOS?

</PropertyGroup>
Comment on lines +72 to +90
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@radekdoulik radekdoulik Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Copy link
Member

@ilonatommy ilonatommy Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think copilot does not want to work on PRs that were not opened by him after assigning him to an issue. At this point, I cannot even assign this PR to him.


<PropertyGroup Condition="$([MSBuild]::IsOSPlatform('windows'))">
<ChromeDirName>chrome-win</ChromeDirName>
<ChromeDriverDirName>chromedriver_win32</ChromeDriverDirName>
Expand Down Expand Up @@ -109,8 +129,8 @@
AfterTargets="$(WasmProvisionAfterTarget)"
Condition="(!Exists($(ChromeStampFile)) or !Exists($(ChromeBinaryPath))) and '$(InstallChromeForTests)' == 'true'">

<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'))"
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
<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'))"

Copilot uses AI. Check for mistakes.
Text="Chrome provisioning only supported on Linux, Windows, and macOS." />
<Error Condition="'$(ChromeVersion)' == ''"
Text="No %24(ChromeVersion) set. This can be set in eng/testing/BrowserVersions.props" />

Expand All @@ -137,8 +157,8 @@
AfterTargets="$(WasmProvisionAfterTarget)"
Condition="(!Exists($(ChromeDriverStampFile)) or !Exists($(ChromeDriverBinaryPath))) and '$(InstallChromeForTests)' == 'true'">

<Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows'))"
Text="ChromeDriver provisioning only supported on Linux, and windows." />
<Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows')) and !$([MSBuild]::IsOSPlatform('macos'))"
Text="ChromeDriver provisioning only supported on Linux, Windows, and macOS." />
<Error Condition="'$(ChromeVersion)' == ''"
Text="No %24(ChromeVersion) set. This can be set in eng/testing/BrowserVersions.props" />

Expand All @@ -164,8 +184,8 @@
AfterTargets="$(WasmProvisionAfterTarget)"
Condition="(!Exists($(V8StampFile)) or !Exists($(V8BinaryPath))) and '$(InstallV8ForTests)' == 'true'">

<Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows'))"
Text="V8 provisioning only supported on Linux, and windows." />
<Error Condition="!$([MSBuild]::IsOSPlatform('linux')) and !$([MSBuild]::IsOSPlatform('windows')) and !$([MSBuild]::IsOSPlatform('macos'))"
Text="V8 provisioning only supported on Linux, Windows, and macOS." />
Comment on lines +187 to +188
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
<Error Condition="'$(V8Version)' == ''" Text="%24(V8Version) not set" />
<Error Condition="'$(_V8PlatformId)' == ''" Text="%24(_V8PlatformId) not set, needed for constructing the snapshot url." />

Expand Down
Loading