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 Apple Silicon builds #88

Merged
merged 1 commit into from
Aug 9, 2022
Merged

Fix Apple Silicon builds #88

merged 1 commit into from
Aug 9, 2022

Conversation

Chris--B
Copy link
Contributor

@Chris--B Chris--B commented Aug 6, 2022

Running

$ cargo build --target aarch64-apple-darwin

was failing with link errors because the ObjC code in SDL was being built for x86_64 for some reason. Just objective C. C was fine. Weird, right?
Setting some CMake variables keeps everything aligned with the right architecture though! Hurray!

This also does two related things, but I can cut them from the PR if needed:

  1. Disables SDL_SHARED if the dynamic_link feature is not enabled. This is broken on iOS and I don't know why. But without this feature, nothing should need it right? Windows breaks. Your guess is as good as mine.
  2. Adds some iOS checks to build.rs. With this I can get iOS building, but it's still untested so understandable if we don't want this in main yet. See my comment here. Removed. I'm confused by parts of it and will come back when I have code running.

@Lokathor
Copy link
Owner

Lokathor commented Aug 6, 2022

i have no ios knowledge at all, and you're my new ios expert ;P

@Chris--B
Copy link
Contributor Author

Chris--B commented Aug 6, 2022

Okay Windows is unhappy with something. I wish the logs had -vvv on them, so we could see what CMake is doing.
My guess, though, is the SDL_SHARED change (somehow...), since that's the only change that's not guarded by a macOS check.

I'll remove that and see what it does. 🤷‍♀️

@Lokathor
Copy link
Owner

Lokathor commented Aug 6, 2022

i think in the CI under command: test you can add an args: line and then put the -vvv and it should give the super verbose output

@Lokathor
Copy link
Owner

Lokathor commented Aug 6, 2022

or for cmake, maybe the crate has a way to pass that?

https://docs.rs/cmake/latest/cmake/struct.Config.html#method.build_arg maybe?

@Chris--B
Copy link
Contributor Author

Chris--B commented Aug 6, 2022

I think the -vvv in the workflow is the right idea. CMake spits out enough already, but cargo hides most of it. It prints the link command that failed, but I'm curious about what CMake was doing before that. I have no idea why setting SDL_SHARED=OFF breaks Windows builds, but PR's green so that must have been it.

build.rs Outdated
@@ -57,6 +57,29 @@ fn main() {
cm.define("SDL_SHARED", "ON");
cm.define("SDL_STATIC", "ON");
cm.define("HIDAPI", "ON");

if cfg!(target_os = "macos") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is super unintuitive, but makes sense if you spend enough time in build.rs scripts.
cfg!(target_os = "...") is referring to the target OS of the build.rs script, and then later when we read env::var("CARGO_CFG_TARGET_OS"), that's the target OS of the crate being built. cfg!() reads variables at compile time (of build.rs), while env::var() reads them from whenever it's run.

So this is all skipped if you try to compile for mac/ios on a not-mac platform. Which is rare but something that I think is easy enough to support. I may want to re-do this logic to accompany that... or at least put that disclaimer in a comment. x.x

Copy link
Owner

Choose a reason for hiding this comment

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

ohhhhhhhh

Copy link
Owner

Choose a reason for hiding this comment

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

yes please update that and comment why it's using env and not cfg

probably this is why cross-build always seems broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tidied this up, but it also seems like something cmake-rs could figure out. I'm curious why they do not....

@Chris--B
Copy link
Contributor Author

Chris--B commented Aug 8, 2022

I'm a little confused about building for iOS with CMake, so I cut that from this PR.
rustc and cmake-rs both do Weird Things:tm: when targeting iOS/etc, so I want to know why before I mess with it too much.
See:
rust-lang/cmake-rs#110
rust-lang/rust#73903

Running
```
$ cargo build --target aarch64-apple-darwin
```
was failing with link errors because the ObjC code in SDL was being
built for x86_64 for some reason. Setting this CMake variable keeps
everything aligned with the right architecture though! Hurray!
@Lokathor
Copy link
Owner

Lokathor commented Aug 8, 2022

So, should I merge it as is, or is there more to do?

@Chris--B
Copy link
Contributor Author

Chris--B commented Aug 8, 2022

I'm happy with this as-is. It fixes a minor pain point on ASi macos and doesn't appear to break anything. Any follow up I do for iOS should be separate.

So if you're happy with it, go ahead and merge.

@Lokathor Lokathor merged commit 2d662d8 into Lokathor:main Aug 9, 2022
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