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

Straightening up messy Apple compilations #664

Closed
dot-asm opened this issue Mar 14, 2022 · 24 comments
Closed

Straightening up messy Apple compilations #664

dot-asm opened this issue Mar 14, 2022 · 24 comments
Labels
O-apple Apple targets and toolchains

Comments

@dot-asm
Copy link
Contributor

dot-asm commented Mar 14, 2022

I would like to make a case for instead of endlessly struggling to map all those Apple-specific environment variables, e.g. like in #661 and #662, one would simply let xcrun do all the work. I mean instead of calling composing all sorts of command line options and passing them to cc, one would simply call xcrun -sdk [iphoneos|iphonesimulator|watchos|etc.] cc -arch [arm64|x86_64|armv7]. In other words, cc-rs would have to simply map Rust target triplet to a combination of -sdk and -arch parameters.

This was referenced Mar 14, 2022
@dot-asm
Copy link
Contributor Author

dot-asm commented Mar 15, 2022

Moving #661 (comment) here. @indygreg, @dvc94ch, @vladimir-ea.

Tangentially related to this, I've implemented Apple SDK discovery in pure Rust in https://github.com/indygreg/PyOxidizer/blob/ae848c9083c9c8563d5890f5d39a8e04a1d71832/tugger-apple/src/sdk.rs. This includes parsing the SDKSettings.json files to extract SDK metadata such as versions and which targets are supported. Like xcode-select and xcrun, it supports honoring DEVELOPER_DIR for finding the install root of SDKs. There's also some related code at https://github.com/indygreg/PyOxidizer/blob/ae848c9083c9c8563d5890f5d39a8e04a1d71832/pyoxidizer/src/environment.rs#L531 (and elsewhere in this file) for attempting to find an appropriate SDK to use given constraints. (This code should arguably be moved to the aforementioned crate/module.)

This code probably isn't suitable for inclusion in this crate as-is. But if you want me to extract it to its own crate or incorporate it into cc-rs somehow, let's talk (in another issue presumably).

As for this PR, externally specifying SDKROOT to force the Apple SDK to use seems reasonable to me. This is what I've done with PyOxidizer to allow users to override the built-in/default SDK finding. I say this without knowing how the Rust compiler internally handles SDKROOT, so I may be missing some important context! But my understanding is that build tools commonly export SDKROOT to force use of a specific SDK to avoid downstream tools performing their own SDK discovery.

@dot-asm
Copy link
Contributor Author

dot-asm commented Mar 15, 2022

But if you want me to extract it to its own crate or incorporate it into cc-rs somehow, let's talk

First of all I want to clarify that I'm just a cc-rs user responding to #663. Well, obviously in my own way, but nevertheless. #663 [among other things] effectively reads "keep it manageable" and I can't agree more.

@dot-asm
Copy link
Contributor Author

dot-asm commented Mar 15, 2022

externally specifying SDKROOT to force the Apple SDK to use seems reasonable to me.

xcrun respects externally specified SDKROOT, so what's the problem? It "kicks in" only when you don't specify one and finds one suitable for --sdk [iphoneos|phonesimulator|watchos|etc.]

I say this without knowing how the Rust compiler internally handles SDKROOT,

It doesn't matter for cc-rs. Its job is to compile suitable .o module, that's it.

@dot-asm
Copy link
Contributor Author

dot-asm commented Mar 15, 2022

Tangentially related to this, I've implemented Apple SDK discovery in pure Rust.

And the question in the context would be what would it take to implement xcrun replacement? Would it make more sense than to make cc-rs less manageable?

[Recall that the "tangent" effectively is "cross-compiling on non-MacOS host."]

@dot-asm
Copy link
Contributor Author

dot-asm commented Mar 15, 2022

xcrun respects externally specified SDKROOT, so what's the problem?

This might be formally incorrect or misleading statement. xcrun "respects" SDKROOT only if you don't specify --sdk flag. In other words, xcrun cc ... sets SDKROOT to MacOSX.sdk only if SDKROOT is not set, while xcrun --sdk macosx cc ... always sets SDKROOT to MacOSX.sdk.

@dvc94ch
Copy link
Contributor

dvc94ch commented Mar 16, 2022

Don't think there is much point discussing this before #663 is resolved. I don't think this solution conflicts with the fairly simple and straightforward setting an environment variable solution.

@imWildCat
Copy link

@dvc94ch now #663 seems to be resolved. Could we move on this discussion?

I'm happy to do whatever I can to help the iteration on mobile platforms.

@dvc94ch
Copy link
Contributor

dvc94ch commented May 19, 2022

So to summarize the behaviour of xcrun (what @indygreg said):

  1. if SDKROOT is set return that
  2. if DEVELOPER_DIR is set select an sdk from developer dir
  3. use default paths from xcode to find an sdk

On linux/windows we can then set the SDKROOT or DEVELOPER_DIR and it should just work. Extracting that logic into a crate seems reasonable. I don't think we need a binary called xcrun if the resulting behaviour is the same (use same environment variables). Not sure what the policy is of cc-rs on external crates, maybe it needs to be added to cc-rs.

@jurvis
Copy link

jurvis commented May 23, 2022

+1 will love to see this change landed. let me know how I can help!

(rust-bitcoin/rust-secp256k1#443)

@dvc94ch
Copy link
Contributor

dvc94ch commented May 23, 2022

This code probably isn't suitable for inclusion in this crate as-is. But if you want me to extract it to its own crate or incorporate it into cc-rs somehow, let's talk (in another issue presumably).

@indygreg does your offer to factor out the sdk selection logic into a separate crate still stand?

@indygreg
Copy link

What do you think about https://github.com/indygreg/toolchain-tools/tree/main/apple-sdk?

This is effectively what exists in tugger-apple before - just in its own crate with slightly better error handling and optional dependencies to jettison the JSON and plist parsing for people who don't need it.

It still needs support for SDKROOT handling. Want to send a PR?

@indygreg
Copy link

... and I guess I need to port some of PyOxidizer's code for finding an appropriate SDK to use. That should be easy enough. But it may not happen for a few days since I'm busy this week.

I also have to think about how to do targeting without dependency bloat. Right now we pull in a few dozen crates (via plist and serde_json) so we can parse SDKSettings.json and SDKSettings.plist. You need to parse these files in order to do SDK selection / targeting properly. But I imagine there's a simpler/faster solution based purely on parsing version components in directory names that could be employed instead. It isn't as robust but it likely gets the job done.

@indygreg
Copy link

I significantly refactored https://github.com/indygreg/toolchain-tools/tree/main/apple-sdk today. I think it is close to being generally usable. The SdkSearch type should be usable as a generic - yet powerful - mechanism for finding and filtering SDKs.

My plan is to port PyOxidizer to use this crate. Once I'm convinced the API feels reasonable, I'll publish a 0.1 release on crates.io.

If you build the crate without default features, it doesn't have any dependencies. You lose out on anything that needs to parse SDKSettings.json or SDKSettings.plist. But for cc-rs's use case, I think this will be fine.

Please play around with the crate and let me know if you have any requests.

@indygreg
Copy link

And version 0.1 is published! https://crates.io/crates/apple-sdk

@dvc94ch
Copy link
Contributor

dvc94ch commented May 26, 2022

@indygreg Haven't tried it yet, but looks very well documented and tested. Thank you!

@dot-asm
Copy link
Contributor Author

dot-asm commented Jun 1, 2022

Once again,

#663 [among other things] effectively reads "keep it manageable" and I can't agree more.

And even though there were changes, they don't appear for the better. There is not a single member of the Rust organization on the watchers' list, and I see no signs that we are not still treading water... This is not to diminish @indygreg's contribution, not at all! But I'd still argue in favour of letting xcrun, or stand-in on non-MacOS hosts, do the weight lifting.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jun 1, 2022

So basically apple-sdk needs a bin called xcrun or cc-rs needs to replace their sdk selection logic with apple-sdk. I guess either option works, we really need to hear from maintainers here. cc @m-ou-se

@indygreg
Copy link

indygreg commented Jun 2, 2022

I'm fine with any outcome here. Shoring up the logic for apple-sdk is something I've been wanting to do for a while anyway. So if cc-rs doesn't use apple-sdk, I won't take offense or anything like that.

I'll just sit tight and assess.

@colincornaby
Copy link

colincornaby commented Aug 5, 2022

Ran across this thread working through some issues in cc-rs. For Catalyst - cc-rs is still passing an old version of the build triple (noted in #678). The Bitcode flag is also being passed for Catalyst - even though Catalyst shouldn't use Bitcode. There's also the lurking elimination of Bitcode on Apple platforms (although I think Bitcode objects should compile fine into non-Bitcode targets.)

Catalyst output basically isn't usable right now. Is there any forward progress with this PR, or would continued patches to the existing implementation be welcome?

@thomcc
Copy link
Member

thomcc commented Oct 25, 2022

I'm concerned that using xcrun for everything would prevent the ability to use alternative compilers and compiler wrappers (either sccache or custom scripts that wrap cc and the like). The environment variable wrangling does need cleaning up though, as does the situation around bitcode.

@thomcc thomcc added the O-apple Apple targets and toolchains label Oct 26, 2022
@thomcc
Copy link
Member

thomcc commented Oct 26, 2022

would continued patches to the existing implementation be welcome?

Yes.

@thomcc
Copy link
Member

thomcc commented Oct 26, 2022

I don't know that we'd want to use apple-sdk outright, it has a lot of fairly heavyweight dependencies that we'd probably rather avoid adding unconditionally (this is more important for us than might normally be expected, as cc-rs blocks the build phase of a staggering number of crates in the ecosystem).

That said, I'll do some investigation, it's possible those could be removed more easily, or the logic adapted in a way that's viable for us.

@indygreg
Copy link

I don't know that we'd want to use apple-sdk outright, it has a lot of fairly heavyweight dependencies

apple-sdk has 0 dependencies if default features are disabled.

For cc-rs's use case of simply locating the latest version of an AppleSDK, the optional parse feature (which reads JSON and plist files in order to read metadata within) is likely not needed. However, the parse feature can be useful for stronger validation, such as ensuring a given SDK supports targeting a given OS version.

If cc-rs or other crates wanted the advanced functionality without the crate bloat, I could likely trim the dependency tree of apple-sdk. Let me know.

@thomcc
Copy link
Member

thomcc commented Oct 26, 2022

apple-sdk has 0 dependencies if default features are disabled.

Oh! Thanks. That wasn't clear to me from docs.rs (which just lists the dependencies, and not that they are optional), and I couldn't immediately find the source from the repo link listed on crates.io, but I didn't look very hard (I was mostly focused on triage and cleanup of cc's PR list last evening).

For cc-rs's use case of simply locating the latest version of an AppleSDK, the optional parse feature (which reads JSON and plist files in order to read metadata within) is likely not needed ... If cc-rs or other crates wanted the advanced functionality without the crate bloat

I'll look through it, but what you say is likely correct, and I suspect that cc-rs doesn't need the optional functionality.

@dot-asm dot-asm closed this as completed Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-apple Apple targets and toolchains
Projects
None yet
Development

No branches or pull requests

7 participants