Change linking of CGDisplayCreateUUIDFromDisplayID on macos #1626
Conversation
|
This is what I also tried myself. But it looks like ColorSync is only available in 10.13+. I've been using 10.11 in my cross compile scripts, so that did not fix the issue for me. I guess I can upgrade myself easily, but I don't know if winit aims to only support latest versions of MacOS or no. |
francesca64
added
DS - macos
C - waiting on maintainer
|
@Imberflur thanks for the PR, and sorry for the delay! So, if I'm understanding correctly, However, the existing code has worked fine for several people in my company on both Mojave (10.14) and Catalina (10.15). It smells like macOS is more liberal with this than your cross compilation setup is. I'm not sure if it'd be fair to consider that a bug on your end, since there might not be anything you can realistically do about that? The changes you made are strictly correct either way, so no worries.
It looks like this PR would fix cross-compilation for users on 10.13+ at the expense of making winit completely unusable for anyone below that, which isn't ideal. However, I'm probably comfortable with that, pending any counterarguments.
High Sierra (10.13) came out in 2017, which is a fairly long time ago by Apple standards. I think it'd be a reasonable minimum, especially since older versions are unlikely to receive much testing, so we wouldn't be able to make any support guarantees anyway. Would you be able to use a more recent version? |
|
@chrisduerr do you know if many Alacritty users are below macOS 10.13? I don't want to be unpopular. |
|
It seems strange to force a breaking change like this when macOS is likely backwards compatible in some way. At least it has worked great for us, might be related to the Of course I do not know how many users are below macOS 10.13, since we have no intention to track that. I haven't seen any issues with a reported macOS version that low for a while, but people very rarely report their macOS versions in issues they create unfortunately. |
|
Since actual macOS systems appear to be far more lenient with this, I wonder if there is some way we could bring that into the cross-compilation setup? |
francesca64
added
C - needs investigation
I think @chrisduerr gave you a good lead / basis for comparison there. I can't investigate this myself, but I wish you the best of luck in finding a solution. We can revisit merging this if it turns out to be intractable or infeasible, but I'm quite optimistic. |
|
I wonder if there's some way to check for things in build.rs and conditionally enable them? But I'm not sure that build.rs it that flexiable(that's how you do such things in C world with checking whether small programs could be compiled and set proper defines, etc). That could be a nice direction to investigate. |
|
@kchibisov I'm worried that'd end up being a lot of complexity. It's definitely worth investigating if we end up needing to move forward with this, though. |
In this case, maybe its worth asking in osxcross repo since that is what both me and @Imberflur use, it seems? I don't know much about MacOS and how this all works and why is it different but maybe they do.
Yea, I'm using this patch and updated to 1.13 now in my scripts with success |
|
It looks like we are already setting the deployment target to a low value https://gitlab.com/veloren/veloren-docker-ci/-/blob/master/base/Dockerfile#L64 I think the issue may be with the SDK we are using. I'll probably need to wait for others to investigate this since I'm not the maintainer of our CI. @kuviman are you getting the SDK for cross compilation from the same source? |
|
Well, if we want to support old and a new platform winit can check the target we're compiling to in build.rs and pass proper cfg to rust, and in the code we'll just winit can also define build time env variable like |
|
Just fyi, you can utilize conditional compilation with cargo in build.rs. So you can define a custom cfg in build.rs script and use in macOS/ios code. The basic idea looks like alacritty/crossfont@7134cd4 . So in build.rs you should check if the thing you want to use is presented, assuming that your target platform is macos, which cargo will pass to you via env variable I'm just leaving it here, if your cross issue is still relevant, since solution like that should work. I'm not entirely sure how to check for target macos version automatically though, but adding winit specific env variable for build.rs could be valuable solution, but I'd prefer more automatic approaches to solve that problem. |
|
Maybe something like this? // build.rs
fn main() {
// If building for macos >=10.13 use CGDisplayCreateUUIDFromDisplayID from ColorSync instead of CoreGraphics
if std::env::var("CARGO_CFG_TARGET").as_deref() == Ok("macos") {
if let Ok(target) = std::env::var("MACOSX_DEPLOYMENT_TARGET") {
let mut split = target.split('.').map(|s| s.parse::<u32>().ok());
if let Some(major_version) = split.next().flatten() {
let minor_version = split.next().flatten().unwrap_or(0);
if (major_version, minor_version) >= (10, 13) {
println!("cargo:rustc-cfg=use_colorsync_cgdisplaycreateuuidfromdisplayid");
}
}
}
}
}Personally I would be fine with more manual solutions. |
That seems about fine for me. |
Imberflur
force-pushed
the
macos-linking
branch
from
b1cfa01
to
40cd627
Compare
|
I tried this out, however we weren't already setting I think this might be because we are setting the min version to something else when setting up the cross compiler: So I think it would be easier and less hazardous to just have a winit specific env var. Especially, since the original linking error only occurs during cross compilation. |
Imberflur
force-pushed
the
macos-linking
branch
from
40cd627
to
dc8d051
Compare
|
I changed it to a simple manually configurable env var. I avoided using the macos target version as the input for the env var since from my previous tests in our cross compilation setup the version we are targeting doesn't align with what conclusions we can reach from the documentation about the availability of the ColorSync API. edit: confirmed using this env var works in our CI |
Imberflur
force-pushed
the
macos-linking
branch
2 times, most recently
from
c4378b8
to
263ba1a
Compare
Imberflur
force-pushed
the
macos-linking
branch
from
263ba1a
to
e994dd9
Compare
|
Added a note to the readme |
There was a problem hiding this comment.
Great! @francesca64 if you give your blessing, I think we can merge this.
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ColorSync (which AppKit uses internally) available to us (on macOS 10.13 or above). Lately something changed in rustc so that this no longer works, see rust-lang/rust#91372. So instead, we link to AppKit directly within the `winit` crate, which, for now, allows us to use the private symbol.
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ColorSync (which AppKit uses internally) available to us (on macOS 10.13 or above). Lately something changed in rustc so that this no longer works, see rust-lang/rust#91372. So instead, we link to AppKit directly within the `winit` crate, which, for now, allows us to use the private symbol.
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us (on macOS 10.8 or above). However, this does not work on macOS 10.7; instead, we should just link to ApplicationServices directly!
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us (on macOS 10.8 to 10.13). However, this does not work on macOS 10.7 (where AppKit doesn't link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.
See also #1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.
See also rust-windowing#1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.
See also #1626. The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.
This fixes an issue where cross compilation to macos was failing in our CI.
the error we were getting is
The CI is setup using
10.13from https://github.com/phracker/MacOSX-SDKsThe relevant docker file is here https://gitlab.com/veloren/veloren-docker-ci/-/blob/master/base/Dockerfile
The cross compilation was setup using this guide https://wapl.es/rust/2019/02/17/rust-cross-compile-linux-to-macos.html
I don't know enough to tell whether this change will impact regular users. However, we have had two macos users able to build with this change without any issues.
Please let me know if there is a better way to fix this 🙂
cargo fmthas been run on this branchcargo docbuilds successfullyCHANGELOG.mdif knowledge of this change could be valuable to users