Skip to content

Conversation

@maxded
Copy link
Contributor

@maxded maxded commented Apr 24, 2025

This change removes the pregenerated bindings and instead always generates the bindings at compile time. This results in less work needed to support all targets from cpuinfo.

With bindings being generated at compile time, the cross compilation CI tests now also validate the bindings. Before it would always test against the windows bindings which would "succeed" even though its ABI compared against the compiled cpuinfo static library was incorrect. To simplify this CI flow, I've decided to move the cross compilation checks into the general ci.yaml workflow.

I've also patched up our higher level rust code (lib.rs) to incorporate platform and target specific fields in cpuinfo.h. As a result, this crate now properly supports all targets from cpuinfo.

@maxded maxded changed the title generate bindings Generate bindings for multiple targets Apr 24, 2025
@maxded maxded changed the title Generate bindings for multiple targets Support all targets Apr 24, 2025
@maxded maxded marked this pull request as ready for review April 24, 2025 14:02
@maxded maxded requested a review from MarijnS95 April 24, 2025 14:02
Comment on lines -17 to -18
- name: Cargo test
run: cargo test --workspace
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this test here as it is already covered by another build step for all targets.


on:
push:
pull_request:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the pull_request one as it would trigger our CI jobs twice which isn't needed.

@maxded
Copy link
Contributor Author

maxded commented Apr 24, 2025

The segmentation faults experienced on aarch64-apple-darwin have been resolved with these changes!

Comment on lines +82 to +86
# - target: armv7-unknown-linux-gnueabihf
# os: ubuntu-latest
# name: Linux ARMv7
# cross: true
# test: true
Copy link
Contributor Author

@maxded maxded Apr 24, 2025

Choose a reason for hiding this comment

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

Mostly a cross compile issue, not an actual issue with the code so removing the test step for now.

https://github.com/Traverse-Research/cpuinfo-rs/actions/runs/14644308875/job/41094360680

@MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 changed the title Support all targets Regenerate header bindings for the current target instead of reusing incompatible x64 Windows bindings Apr 24, 2025
MarijnS95

This comment was marked as resolved.

@maxded maxded requested a review from MarijnS95 April 24, 2025 17:49
let uarch_info = unsafe { cpuinfo_get_uarch(i) };
infos.push(unsafe {
#[cfg(target_arch = "x86_64")]
#[cfg(any(target_arch = "x86_64", target_arch = "x86"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarijnS95 I've also added the 32 bit targets similarly to how cpuinfo does it.

infos.push(unsafe {
#[cfg(any(target_arch = "x86_64", target_arch = "x86"))]
let cpuid = Some((*uarch_info).cpuid);
#[cfg(all(not(target_arch = "x86_64"), not(target_arch = "x86")))]
Copy link
Member

Choose a reason for hiding this comment

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

Nit: not(any()) makes this more similar (to read/copy-paste) with the above case :)

maxded and others added 2 commits April 25, 2025 10:04
Co-authored-by: Marijn Suijten <[email protected]>
@maxded maxded merged commit 6618512 into main Apr 25, 2025
9 checks passed
@MarijnS95 MarijnS95 deleted the generate-bindings branch April 25, 2025 08:29
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.

3 participants