rust-lang / cc-rs

Rust library for build scripts to compile C/C++ code into a Rust library
https://docs.rs/cc
Apache License 2.0
1.84k stars 442 forks source link

Parse default Apple deployment target from SDK properties #1009

Open BlackHoleFox opened 7 months ago

BlackHoleFox commented 7 months ago

This implements a slimmed-down version of clang's default version parsing to obtain default deployment targets in Apple's ecosystem. As opposed to using xcrun, this gets a correct value in all XCode installations and should be more reliable since its the same thing clang does (which we emulate in some places).

It also means compilation for Apple targets on non-macOS hosts (with the right SDK) should get the right version again too (no more xcrun), even if that's not officially supported.

Closes https://github.com/rust-lang/cc-rs/issues/1001 and https://github.com/rust-lang/cc-rs/issues/963

BlackHoleFox commented 7 months ago

@NobodyXu Yeah those could work. I was trying to keep the build time for cc down by going for a minimally-compliant JSON approach.

For a cursory review tinyjson is 1008 lines of source and json-deserializer 507. However json-deserializer doesn't handle Unicode at all so it seems like a bad choice if cc ever needs to read something else that isn't XCode SDKs.

For build times, it looks like the vendored JSON implementation is the fastest. For reference main takes 0.7s total to build on my M1 Max laptop according to cargo build --timings. tinyjson adds 0.15s of time to the build. smoljson's minimal vendoring is ~3x faster to build it seems. tinyjson ends up becoming ~36% of the crates build time :rip:

Here's main:

image

and the with the JSON handlers:

JSON Handling Library cc Build Time main delta Graph
tinyjson 0.9s (N=3) ~0.15s image
Vendored smoljson. 0.6s (N=3). ~0.05s image
NobodyXu commented 7 months ago

Thanks, so the in-house json implementation is actually the fastest.

@BlackHoleFox Would it be possible to parse them before build and generate a .rs file, like https://github.com/rust-lang/cc-rs/pull/1004 does?

I know that it's probably a bit hard and might be impossible since the DefaultDeploymentTarget seems to be coupled with the sdk version they actually use.

NobodyXu commented 7 months ago

@BlackHoleFox If that's not possible, then I suggest further simplify the json parser by cutting unused features.

From the code, it seems that only Reader::read_str_from_object is used, with nesting for macOS_iOSMac.

I'd suggest remove all code except for the one handling objects, null and string, since we don't care about arrays or other stuff.

I'd suggest parsing the entire json file into a

enum Value<'json> {
    Object(BTreeMap<&'json str, Value>),
    String(&'json str),
    Null,
}

Using &str is ok, because the key and value we want does not have escape characters. Any element that have escape characters, can be just ignored and not handled at all.

We also don't need validation of json, we don't care about that, the json provided by macOS sdk should always be valid. As long as we can get DefaultDeploymentTarget and VersionMap.macOS_iOSMac, we don't care about other fields (for now).

Parsing the entire file into a Value like that, would also prevent the need to parse the json twice.

BlackHoleFox commented 7 months ago

@NobodyXu I can trim it down further, yeah. For string escaping though, I disagree with removing support for that. We can have a minimally-useful JSON parser in cc-rs but escapes are part of the specification so I think they should stay. There are even other fields in the SDK JSON that have escapes.

Do you really think its worth making it basically nothing just to assume the XCode JSON is valid?

NobodyXu commented 7 months ago

For string escaping though, I disagree with removing support for that. We can have a minimally-useful JSON parser in cc-rs but escapes are part of the specification so I think they should stay. There are even other fields in the SDK JSON that have escapes.

I'm ok keeping the string escapes, I just want to keep cc-rs maintainable.

Last time I tried introducing a jobserver impl into cc-rs and I ended up removing it, so I prefer the json implementation to be simple.

So how about parsing the json into:

enum Value<'json> {
    Object(BTreeMap<Cow<'json, str>, Value>),
    String(Cow<'json, str>),
    Boolean(bool),
    Null,
}

If we ever need to get more fields (e.g. array, integer, float) then we can add that in future, though I'm pretty sure we won't ever need to parse float.

BlackHoleFox commented 7 months ago

I can certainly try, but I'm bad at writing parsers so we will see how well "parse this blob into an ::Object" goes.

cavivie commented 7 months ago

@NobodyXu BTW, do we think that SDKSettings.json always exists in the sdk root path? If not, do we fallback to the xcrun method (xcrun --show-sdk-version), or is there a flag (such as a variable) to control which method cc-rs is forced to use? At the same time, having the control flag means that when we make a mistake in judgment, we can allow downstream to fix the current build faster without having to wait for the cc-rs repair patch.

NobodyXu commented 7 months ago

That's a really good question that I have no answer with.

cc @BlackHoleFox

BlackHoleFox commented 7 months ago

Yes they always exist inside XCode and it's CommandLineTools variants even past what stable Rust supports.

If you use a weird/mangled build environment you can just set the deployment target manually and this codepath will be skipped.

BlackHoleFox commented 6 months ago

@NobodyXu I wasn't up to being able to figure out the nested object parsing, apologies.

I did trim down more unused stuff from the JSON parser though so its now <500 lines. I don't really think this code will need touched but if you want to have a try at this feel free. I don't think removing types from the parsers capabilities simplifies anything though because you still have to know what integers etc look like to skip/error on them.

NobodyXu commented 6 months ago

@NobodyXu I wasn't up to being able to figure out the nested object parsing, apologies.

I did trim down more unused stuff from the JSON parser though so its now <500 lines.

Thank you, cc @thomcc do you have any idea on how we can go further on simplying this json parser?

NobodyXu commented 6 months ago

So maybe we could resolve both of these concerns by shelling out to plutil? E.g. the following can be used to extract a version from the version map:

According to the man page of plutil it's only available in macOS 10.2 and later:

The plutil command first appeared in macOS 10.2.

But it indeed has better support than SDKSettings.json

BlackHoleFox commented 6 months ago

Huh indeed, I guess I miscounted supported macOS host versions to XCode releases when I started on this PR.

I will start on replacing this with plutil since that's what people want but this is going to add an external binary dependency for anyone cross-compiling from Linux/BSD -> macOS. It looks like Debian, Arch, etc all have plutil somewhere though. I guess they already have enough edge cases though, maybe?

BlackHoleFox commented 6 months ago

They could also just set MACOSX_DEPLOYMENT_TARGET in their build env to skip this parsing but no one actually does that in practice and we can't break that (which is why this PR exists).

BlackHoleFox commented 6 months ago

It looks like Debian, Arch, etc all have plutil somewhere though.

This actually seems like its a badly written replacement for plutil and not a complete alternative. All the man pages don't have the extract flag anywhere. If we force people cross-compiling to set MACOSX_DEPLOYMENT_TARGET this becomes a non-issue though. What do you think @NobodyXu? @thomcc. I have the plutil changes made locally and can push them up if we agree on a line in the sand for cross-compilers.

NobodyXu commented 6 months ago

If we force people cross-compiling to set MACOSX_DEPLOYMENT_TARGET this becomes a non-issue though.

The question is if anyone does cross compiling without setting it though.

I don't have any data for it.

thomcc commented 6 months ago

I think it's fine to ask them to set it when cross-compiling. I'd care if we broke it without a way forward (e.g. if we had a hard dep on plutil without an env var workaround), but this seems fine to me.

NobodyXu commented 6 months ago

plutil seems to be only available in macOS 10.2 and later man page of plutil:

The plutil command first appeared in macOS 10.2.

Maybe we can fallback to the xcrun if plutil's not available?

madsmtm commented 6 months ago

The minimum supported macOS version for rustc is 10.12, so I'd say that's a non-issue.

NobodyXu commented 6 months ago

The minimum supported macOS version for rustc is 10.12, so I'd say that's a non-issue.

Thanks, that's good to hear, though IIRC we got issues submit for macOS older than that, so I'm not 100% sure it is OK. my memory seems to be off, so it should be ok.

MarcusCalhoun-Lopez commented 5 months ago

Please forgive me if I am not fully understanding the issues here. I am far from an expert in Rust, but I help maintain the Rust port in MacPorts.

Might /usr/libexec/PlistBuddy be of use here?


On macOS Sonoma (version 14.4.1), the following all give 14.4:

env SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk xcrun --show-sdk-version

plutil -extract Version raw /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/SDKSettings.plist

/usr/libexec/PlistBuddy /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/SDKSettings.plist -c "print Version"

In addition, both plutil and PlistBuddy give 17.4:

plutil -extract VersionMap.macOS_iOSMac."14\.4" raw /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/SDKSettings.plist

/usr/libexec/PlistBuddy /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/SDKSettings.plist -c "print VersionMap:macOS_iOSMac:14.4"


The advantage to PlistBuddy is that goes back farther than plutil -extract.

On macOS Sierra (version 10.12), the following both give the same result:

env SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk xcrun --show-sdk-version

/usr/libexec/PlistBuddy /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/SDKSettings.plist -c "print Version"

While plutil results in an error. plutil -extract Version raw /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/SDKSettings.plist


While macOS older than 10.12 is not supported by Rust, MacPorts stills attempts to maintain older systems. On Mac OS X Leopard (version 10.5.8), /usr/libexec/PlistBuddy /Developer/SDKs/MacOSX10.5.sdk/SDKSettings.plist -c "print Version" still works and gives 10.5.

NobodyXu commented 5 months ago

That sounds nice, we could use PlistBuddy, and fallback to plutil, or xcrun if the environment does not have it (might be relevant for cross-compiling).

cc @BlackHoleFox @madsmtm

BlackHoleFox commented 5 months ago

I don't think that has an effect on crosscompiling with any fallback, so we should just use one. None of these tools are on Linux etc so I wouldn't expect crosscompilers to have any.

I'm fine with switching to use plistbuddy if it makes someone's life easier (seeming MacPorts?), just need to figure out the syntax to access nested maps (for catalyst versions).

thomcc commented 4 months ago

I think it's fine for us to defer use of plistbuddy to a future PR. (Or, frankly, to not use it at all -- maintenance of this crate is already hard enough without supporting out of date macOS versions...).

NobodyXu commented 4 months ago

That's true, I agree

MarcusCalhoun-Lopez commented 4 months ago

I think it's fine for us to defer use of plistbuddy to a future PR. (Or, frankly, to not use it at all -- maintenance of this crate is already hard enough without supporting out of date macOS versions...).

Please forgive me if I was unclear. I was not trying to suggest maintaining this crate on unsupported OS versions. I was suggesting PlistBuddy is superior to plutil because plutil fails on a supported system, macOS Sierra (version 10.12). I only mentioned older systems to emphasize how far back support for PlistBuddy goes.