denoland / rusty_v8

Rust bindings for the V8 JavaScript engine
https://crates.io/crates/v8
MIT License
3.28k stars 308 forks source link

chore: Fixed incorrect use of cfg! macro and update build.rs to support arm target #1458

Closed Chiichen closed 4 months ago

Chiichen commented 5 months ago

What I've done

  1. Fixed incorrect use of cfg! macro use the CARGO_CFG_TARGET_<OS/ARCH> macro instead of the original cfg! (target_<os/arch>) This is because The build script is compiled for the host architecture as a separate build phase, as that's where it runs. Since the cfg macro runs at compile time it'll always report the host configuration there.When cargo runs the build script it passes the configuration through environment variables, one of which is CARGO_CFG_TARGET_ARCH. Some dicussions can be found here

  2. update build.rs to support arm target support arm target. Like the Aarch64 target, we need to additionally install the cross-compilation toolchain and specify the linker in Cargo/config.toml. I can add this part of the work in next pull request if necessary.

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

mmastrac commented 5 months ago

Is there a reason for the clang submodule update?

Chiichen commented 5 months ago

Is there a reason for the clang submodule update?

It may be related to the fact that I modified the bin path of gn/ninja. I rolled back this modification.

Chiichen commented 5 months ago

Is there a reason for the clang submodule update?

reverted

Chiichen commented 5 months ago

LGTM, waiting on a build pass.

@Chiichen It appears that 4e30365 seems to have caused the apple builds to fail.

Thanks for your reply. It reports in workflow's log at line 135

135 error[E0463]: can't find crate for `core`

This looks like a classic error that can occur when we cross-compile without the corresponding tool chain installed. and I found it at line 7

6 Runner Image
7   Image: macos-14-arm64
8   Version: 20240415.6
9   Included Software: https://github.com/actions/runner-images/blob/macos-14-arm64/20240415.6/images/macos/macos-14-arm64-Readme.md
10  Image Release: https://github.com/actions/runner-images/releases/tag/macos-14-arm64%2F20240415.6

It seems that we are trying to build a x86_64 target on aarch64 platform. I think this may be caused by us not properly installing the Rust toolchain for x86_64-macos on the aarch64-macos device

Actually, I found that the existing workflow does not handle cross-compilation on macos devices. One method is to force it to use x86_64 machines by specifying an older macos version(macos11 or macos12), and the other method is to amend the workflow to support cross-compilation on the macos platform.

Chiichen commented 5 months ago

Details at #1472 .

For now, specifying the runner as macos12 is a workaround for specifying the macos architecture as x86_64. However, macos14 does not guarantee that the running architecture is arm64, so I think the ci workflow on macos needs some improvement.

Chiichen commented 4 months ago

@mmastrac I think the Test error has nothing to do with my modifications. The same error appears at #1473

mmastrac commented 4 months ago

I agree -- we're seeing that error frequently on other PRs. It doesn't make a lot of sense

Chiichen commented 4 months ago

@mmastrac If there is any expected changes that is blocking this PR from being merged into master, I 'm glad to know

mmastrac commented 4 months ago

We should be good to go. Just trying to get that flaky test to pass 😞

Chiichen commented 4 months ago

We should be good to go. Just trying to get that flaky test to pass 😞

Got it. It looks like the test passed ^_^