Update to support ARM64 build target#5246
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
❌ Build CefSharp 147.0.100-CI5487 failed (commit 14883d302f by @jamescharters) |
|
❌ Build CefSharp 147.0.100-CI5488 failed (commit da5271272b by @) |
|
@amaitland Hi Alex, this is currently a draft of changes to get this building for ARM64. My team has compiled this locally and is using it on real ARM64 hardware (M4 Macs) without issue. Was wanting to know how to move forward with this, as it looks like the CI/CD environment does not support this architecture. |
|
❌ Build CefSharp 148.0.90-CI5510 failed (commit fd4a1df55e by @jamescharters) |
| $StartInfo = New-Object System.Diagnostics.ProcessStartInfo | ||
| $StartInfo.FileName = "msbuild.exe" | ||
| $StartInfo.Arguments = $Arguments |
There was a problem hiding this comment.
Is this strictly necessary? From memory there was a specific reason the EnvironmentVariables were cleared (I don't remember the specifics, it's been years since this was done).
Please revert any unnecessary changes to this file. Keeping it to the bare minimum change will also make it easy to review.
| private static IEnumerable<string> GetOptionalDependencies() | ||
| { | ||
| foreach (var dependency in CefOptionalDependencies) | ||
| { | ||
| if (ArchitectureHelper.IsArm64Process && string.Equals(dependency, "d3dcompiler_47.dll", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| yield return dependency; | ||
| } | ||
| } |
There was a problem hiding this comment.
Chromium started shipping the d33compiler some time back.
unless they've stopped shipping the dll this can be reverted.
| case "Arm64": | ||
| case "arm64": |
There was a problem hiding this comment.
Could we not just turn the string to lowercase?
relying on hard coded case comparison seems less than ideal.
If we do need a comparison then using if statements with string comparison to ignore case I think makes more sense then multiple case statements.
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" /> | ||
| <PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" /> |
There was a problem hiding this comment.
If we need this then it will need to be added to the nuspec file.
Until it builds there really isn't much I can do. You'd need to build locally in the mean time. As per my comment inline try reverting your changes to the build script to see if that helps. |
Fixes: [issue-number]
Summary: [summary of the change and which issue is fixed here]
This change introduces full ARM64 support for CefSharp, covering native C++/CLI projects, managed .NET Framework projects, NuGet packaging, and the build pipeline. It enables CefSharp to be built and distributed for Windows ARM64 devices natively.
Changes: [specify the structures changed]
How Has This Been Tested?
Built the solution targeting ARM64 using build.ps1 with -BuildArches arm64 -Target vs2022 on an AMD64 host (cross-compile). Verified arm64 binaries are produced in bin\arm64\Release for both CefSharp.Core.Runtime and CefSharp.BrowserSubprocess.
Screenshots (if appropriate):
Types of changes
Checklist: