Skip to content
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.

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

fix: use CARGO_CFG_TARGET_OS and target_os android #81

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Jan 27, 2025

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 #78.

Copy link

codecov bot commented Jan 27, 2025

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
Copy link
Collaborator Author

mxinden commented Jan 27, 2025

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
Copy link
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...

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
Copy link
Collaborator Author

@mxinden mxinden left a comment

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.

Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
build.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@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
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firefox fails to build on Windows, Android and OSX
2 participants