Skip to content
This repository was archived by the owner on Sep 2, 2025. It is now read-only.

fix: use CARGO_CFG_TARGET_OS and target_os android#81

Merged
larseggert merged 1 commit into
mozilla:mainfrom
mxinden:build-cfg-target-os
Jan 29, 2025
Merged

fix: use CARGO_CFG_TARGET_OS and target_os android#81
larseggert merged 1 commit into
mozilla:mainfrom
mxinden:build-cfg-target-os

Conversation

@mxinden

@mxinden mxinden commented Jan 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

In a build.rs file cfg(target_os = "linux") refers to the target OS of the build.rs binary. The environment variable CARGO_CFG_TARGET_OS refers to the target OS of the final binary.

When cross-compiling, the target OS of the build.rs binary and the target OS of the final binary are not the same. The mtu's build.rs should use CARGO_CFG_TARGET_OS.

In addition, cfg(target_os = "linux") does not include android. This commit cfgs linux and android everywhere.

Fixes https://github.com/mozilla/mtu/issues/78.

@mxinden mxinden mentioned this pull request Jan 27, 2025
Closed
@codecov

codecov Bot commented Jan 27, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.71%. Comparing base (1458645) to head (84ca63e).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main      #81   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files           5        5           
  Lines         557      557           
  Branches      557      557           
=======================================
  Hits          483      483           
  Misses         48       48           
  Partials       26       26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mxinden

mxinden commented Jan 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

While I believe this is a step forward, this still needs more work. I will take another look tomorrow.

I wonder, does the usage of bindgen here even work for cross-compilation in the first place?

Recent build with this pull request: https://treeherder.mozilla.org/jobs?repo=try&revision=d606720cbe371d272648f8d80702d50416962d54&selectedTaskRun=K6LyBVWgR1iof9RZmC43rQ.0

@larseggert

larseggert commented Jan 28, 2025

Copy link
Copy Markdown
Collaborator

I think bindgen should work, if it finds the include files we tell it to ingest in the cross-environment for the target platform. I guess the question is "does it?" for moz-central...

@mxinden
fix(build.rs): use CARGO_CFG_TARGET_OS and target_os android
84ca63e 
In a `build.rs` file `cfg(target_os = "linux")` refers to the target OS of the
`build.rs` binary. The environment variable `CARGO_CFG_TARGET_OS` refers to the
target OS of the final binary.

When cross-compiling, the target OS of the `build.rs` binary and the target OS
of the final binary are not the same. The `mtu`'s `build.rs` should use
`CARGO_CFG_TARGET_OS`.

In addition, `cfg(target_os = "linux")` does not include `android`. This
commit `cfg`s `linux` and `android` everywhere.
@mxinden mxinden force-pushed the build-cfg-target-os branch from c9c5691 to 84ca63e Compare January 28, 2025 18:55
@mxinden mxinden changed the title fix(build.rs): use CARGO_CFG_TARGET_OS fix(build.rs): use CARGO_CFG_TARGET_OS and target_os android Jan 28, 2025
@mxinden mxinden marked this pull request as ready for review January 28, 2025 19:02
mxinden
mxinden commented Jan 28, 2025
View reviewed changes

@mxinden mxinden left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is ready for review.

Comment thread Cargo.toml
Comment thread Cargo.toml
Comment thread build.rs
Comment thread src/lib.rs
@mxinden mxinden changed the title fix(build.rs): use CARGO_CFG_TARGET_OS and target_os android fix: use CARGO_CFG_TARGET_OS and target_os android Jan 28, 2025
@larseggert larseggert enabled auto-merge January 29, 2025 07:33
@larseggert larseggert added this pull request to the merge queue Jan 29, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 29, 2025
@larseggert larseggert merged commit d25c0cf into mozilla:main Jan 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@KershawChang KershawChang Awaiting requested review from KershawChang

@martinthomson martinthomson Awaiting requested review from martinthomson

1 more reviewer

@larseggert larseggert larseggert approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Firefox fails to build on Windows, Android and OSX

2 participants

@mxinden @larseggert