chore: Fixed incorrect use of cfg! macro and update build.rs to support arm target #1458
Conversation
|
|
chiichen
force-pushed
the
fix-build-script
branch
from
36076cf to
48a5ffc
Compare
| @@ -137,20 +137,19 @@ fn build_v8(is_asan: bool) { | |||
| if need_gn_ninja_download() { | |||
| download_ninja_gn_binaries(); | |||
| } | |||
|
|
|||
| let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap(); | |||
There was a problem hiding this comment.
Can you add a few lines here with a note explaining why cfg! isn't appropriate for future contributors to this file? The commentary from the PR description will work.
There was a problem hiding this comment.
Can you add a few lines here with a note explaining why
cfg!isn't appropriate for future contributors to this file? The commentary from the PR description will work.
Of course! Actually #[cfg(...)] attributes don't work as expected from build.rs -- they refer to the configuration of the host system which the build.rs script will be running on. In short, cfg!(target_<os/arch>) is actually the host os/arch instead of target os/arch while cross compiling. Instead, Environment viariables is the officially approach to get the target os/arch in build.rs.
There was a problem hiding this comment.
Maybe some parts of the original code that use the cfg!(target_<os/arch>) macro are used to obtain the host <os/arch>, but I think using this ambiguous syntax is not conducive to long-term maintenance. It's highly recommended to use the HOST environment variable to get the host triplet or use cfg!(target_<os/arch>) with explicit annotation
|
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. |
reverted |
Thanks for your reply. It reports in workflow's log at line 135 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 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. |
|
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. |
|
I agree -- we're seeing that error frequently on other PRs. It doesn't make a lot of sense |
|
@mmastrac If there is any expected changes that is blocking this PR from being merged into master, I 'm glad to know |
|
We should be good to go. Just trying to get that flaky test to pass 😞 |
Got it. It looks like the test passed ^_^ |
What I've done
Fixed incorrect use of cfg! macro use the
CARGO_CFG_TARGET_<OS/ARCH>macro instead of the originalcfg! (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 isCARGO_CFG_TARGET_ARCH. Some dicussions can be found hereupdate 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.