-
Notifications
You must be signed in to change notification settings - Fork 90
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 fixes for Apple M1/x86 #30
base: master
Are you sure you want to change the base?
Conversation
cca5874
to
d92af4f
Compare
@zmwangx Hello. Can you have a look at this? |
d92af4f
to
60365f4
Compare
Hi! Thanks for your submission :) |
60365f4
to
36d6a5d
Compare
3102094
to
c6dc629
Compare
This is still broken in 5.0.0. |
@@ -383,6 +401,19 @@ fn build() -> io::Result<()> { | |||
Ok(()) | |||
} | |||
|
|||
fn os_from_triple(triple: &str) -> &str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there really no way to use rust's target_os!
or something more standard? I'm afraid this will bite us if other architectures start to pop up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust's target definitions are Rust's, and have no guarantee of compatibility with anything, e.g. Rust uses macos
, but the rest of the world uses darwin
.
So yes, it is already broken, and will keep breaking on every new OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to have something that just checks if target_os!
is macos
, and replace it by darwin
if it is? And maybe that does that for other OSes, if it happens for others? Maybe just a fn target_os()
that wraps target_os!
and does the changes or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it's needed for more OSes, but I don't have them to test. Whatever you do, it's going to be an incomplete list that may break on OSes that haven't been tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, but what's the value of CARGO_CFG_TARGET_OS
on macos? Can't you just use a wrapper that gets CARGO_CFG_TARGET_OS and if it's macos, replace it by darwin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CARGO_CFG_TARGET_*
variables are derived from rustc's cfg values, same as cfg!(target_*)
, so they use the same naming scheme.
I can hardcode the result to be correct only on darwin/macos, and leave it incorrect everywhere else, but why should I leave it incorrect everywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my question is just, why do this triple splitting magic (and explode if it doesn't work) instead of just using CARGO_CFG_TARGET
and use a hashmap or something like that to substitute the wrong stuff with whatever you know is right? Or maybe I'm missing the point here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split doesn't work, because Rust's triples are inconsistently named, and sometimes have the "vendor" and sometimes they don't, so you don't know where the OS part starts.
HashMap would work for a closed set. I'm trying to make it work for an open set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point was that maybe we could do a hashmap like {'macos': 'darwin'}
and then just have a custom target_os
function that would just return either the string itself, if it's not in the hashmap? Like
fn target_os(target: &str) -> &str {
let mut os_replacements = HashMap::new();
os_replacements.insert(
"macos".to_string(),
"darwin".to_string(),
);
os_replacements.get(&target).unwrap_or("target")
}
But I feel like we're talking about different things though, so I could be completely wrong 😄
https://github.com/rustsec/rustsec/blob/main/platforms/src/target/os.rs#L112-L134 seems to have a pretty consistent list of OS if we wanna list more stuff here?
Cross-compilation was assuming a setup typical for Linux. Cross-compilation from ARM macOS to Intel macOS has its own quirky requirements, because it uses the same system-global compiler, but needs different flags instead.
Also fixes a bug where
cfg!(target_os)
was used.build.rs
runs on the host OS, so compilation target ofbuild.rs
is always the same as the host.