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

Cross compilation compatibility builds #46

Merged
merged 20 commits into from
Sep 4, 2023

Conversation

Adoni5
Copy link
Contributor

@Adoni5 Adoni5 commented Aug 31, 2023

Main changes

  • After hitting FEATURE: Conditional compilation for aarch64, arm_neon, and simde #13 (comment) a couple of time whilst trying to compile mappy-rs wheels for multiple architectures, I realised that checking target_arch in the build.rs script of minimap2-sys, only considers the host architecture and not the target triple.
    This meant that the target_specific function

    #[cfg(target_arch = "x86_64")]
    fn target_specific(cc: &mut cc::Build) {
    was being run for x86_64 every time, irrespective of which cross compilation was occurring.
    To catch this I have changed the target_specific function and dropped conditional compilation for it based on target architecture, and instead used a check against the TARGET env variable inside the function, adding flags based on whether the target triple is x86 or aarch64.

  • In the minimap2-rs library code, I had to add a From trait to convert presets to C strings for both i8 and u8 types. I also conditionally compile any sections where pointers are passed through to the underlying FFI functions based on architecture. In linux aarch64/armv7 cross compilation, the underlying C compiler expects u8 pointers, whereas linux x86_64, macOS x86_64, and macOS aarch64 expect i8 pointers. I have not been able to test if this breaks native compilation on linux aarch64.

Other changes

  1. For linux aarch64, for some reason to cross compile, the C99 standard had to be enabled, due to for loop constructions in minimap2. I've added this to the simde feature function, https://github.com/Adoni5/minimap2-rs/blob/d804692362e414058123de21d3464f6e7d615f52/minimap2-sys/build.rs#L167
    But I don't know if that's the best place to put it?

  2. I added fakeminimap2 and minimap2-sys as cargo workspaces in the root Cargo.toml, simply so that rust-analyzer would work in the directory. This can be removed if you prefer, but I don't think it has any knock on effects otherwise.

  3. Built fakeminimap2 with local minimap2-rs crate - no reason, just seems semi-logical as it seems useful for testing.

@Adoni5
Copy link
Contributor Author

Adoni5 commented Aug 31, 2023

Final note - the Cargo.toml for minimap2-rs currently points to the local minimap2-sys. This should be updated when the new version of minimap2-sys is up on crates.io

@jguhlin
Copy link
Owner

jguhlin commented Sep 3, 2023

Amazin!. I'm recovering from a conference but will get this pushed out in the next few days.

@jguhlin jguhlin merged commit 3901aa6 into jguhlin:main Sep 4, 2023
2 of 4 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.

2 participants