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

Use SDKROOT when set. #661

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Use SDKROOT when set. #661

merged 1 commit into from
Oct 25, 2022

Conversation

dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented Mar 7, 2022

This fixes #650

@alexcrichton
Copy link
Member

Is there existing precedent for using an environment variable like this? If so is there documentation or precedent that this could link to? (I know nothing of this environment variable myself)

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Mar 7, 2022

well, I don't think there is much precedent for cross compiling ios applications from non macos systems without xcode installed, even though it is certainly possible.

xcode sets the SDKROOT for the target platform and both clang and rust pick it up from there. alternative the sdk root can be provided with --sysroot or -isystem. not sure there is much documentation.

@alexcrichton
Copy link
Member

There's a fair bit of handling of SDKROOT in the Rust compiler around special strings and such, and none of that is mirrored here? That seems like it could cause bugs?

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Mar 8, 2022

well, worst case something doesn't compile. since apple believes everyone runs a mac, sdkroot should always be the target sdk and not the host sdk. this means that rust needs to remove the sdk root when compiling a build script or proc macro. without this change I can't cross compile any crate that uses cc-rs, like for example the objc-exception crate.

@alexcrichton
Copy link
Member

Sorry but I've decided that the current development trajectory of cc is not one that I can maintain, so this crate will need to grow new maintainers before merging.

@dot-asm
Copy link
Contributor

dot-asm commented Mar 14, 2022

I [for one] find all this environment variable chasing tedious and even counterproductive, so I what you would think of #664.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Mar 14, 2022

This PR is explicitly about avoiding xcrun. I understand that that is fine for mac users, but this PR is explicitly about enabling linux users to build ios binaries. This by itself does not make developing ios applications possible from linux, but in conjunction with other efforts like the ones listed below would indeed make linux a suitable host environment for ios developers.

@dot-asm
Copy link
Contributor

dot-asm commented Mar 14, 2022

This PR is explicitly about avoiding xcrun.

Currently xcrun is used to find sdk path and suggestion is to instead use xcrun to fire cc without having to bother with sdk path specifics in cc-rs. As for possible-future non-apple developments, why not aim for xcrun replacement there? I'd say it would be of great benefit for all projects...

@indygreg
Copy link

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.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Oct 25, 2022

cc @thomcc. I see you're the only one watching this crate from the crate maintainers team [0]. So I guess you're the maintainer of this crate now?

@thomcc
Copy link
Member

thomcc commented Oct 25, 2022

Sure, cc is one of the crates I volunteered for, and one I have a vested interest in. Let me take a look.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This looks fine to me -- SDKROOT is a well known environment variable for configuring this (see https://github.com/swift-server/vscode-swift/blob/03f916559c98548ad932a989923a5b264124c8c2/src/toolchain/toolchain.ts#L295 as an example).

I think we'll need to sort out the situation with regards to other env vars (we special case IOS_DEPLOYMENT_TARGET and not others) and bitcode (which doesn't work on catalyst and won't work on watchOS soon), but that can be handled separately.

@thomcc
Copy link
Member

thomcc commented Oct 25, 2022

Apparently I don't have the ability to merge yet, though 😩.

@thomcc
Copy link
Member

thomcc commented Oct 25, 2022

Can you rebase in order to remove the merge commit? I think we want a linear history like we do in the other repos.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Oct 25, 2022

done

@thomcc thomcc merged commit 11f5390 into rust-lang:main Oct 25, 2022
@r-value
Copy link

r-value commented May 23, 2023

Hi, I suspect that picking SDKROOT unconditionally from env has set a wrong sdk_path for cc when targeting {x86_64,aarch64}-apple-ios. Could you please take a look into it?

The bump that triggers the problem: rust-lang/rust#111701
Some log info: rust-lang/rust#111701 (comment)

@dvc94ch
Copy link
Contributor Author

dvc94ch commented May 23, 2023

when targeting ios you should set the SDKROOT to the ios sdk and not the host sdk. if you don't set the SDKROOT it should pick it up automatically.

@r-value
Copy link

r-value commented May 23, 2023

when targeting ios you should set the SDKROOT to the ios sdk and not the host sdk. if you don't set the SDKROOT it should pick it up automatically.

I think the SDKROOT is automatically set somewhere in the ci test and got it wrong for some reason. The code in rustc contains lots of code ignoring SDKROOT if it's clearly wrong.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented May 23, 2023

So set it correctly or don't...

@dvc94ch
Copy link
Contributor Author

dvc94ch commented May 23, 2023

I guess we can check if the sdk root is set correctly and if it isn't ignore it. Seems like the apple toolchain doesn't care?

@r-value
Copy link

r-value commented May 23, 2023

I looked deeper into the ci workflow and found that SDKROOT might be set here:
https://github.com/rust-lang/rust/blob/f3d597b31c0f101a02c230798afa31a36bdacbc6/src/ci/scripts/install-clang.sh#L30-L35
It used a value returned by xcrun and I think that's clearly wrong since we have multiple targets aside from macosx.

FYI the code filtering irrational env was originally introduced in commit rust-lang/rust@fe6d626

@r-value
Copy link

r-value commented May 25, 2023

So set it correctly or don't...

fyi, I found something here: rust-lang/cargo#7283 resolved by rust-lang/rust#64254
Maybe SDKROOT is just wrong in some situation when cross building? tbh I'm not an apple dev so I can't understand the issue quite well.

I guess we can check if the sdk root is set correctly and if it isn't ignore it.

That's exactly what rustc did - perform a sanity check against SDKROOT and ignore it if it's wrong. But I don't have appropriate test environment so I guess we need someone else to implement and test it.

btw, shall we print some warning if the SDKROOT doesn't pass the sanity check? I don't know if it's a common situation that a wrong SDKROOT is automatically set.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented May 25, 2023

Maybe SDKROOT is just wrong in some situation when cross building?

No, it shouldn't be wrong when compiling for the target. All the links you mention are saying that when compiling native parts, target env variables weren't sanitized correctly.

Do build scripts compile in debug mode when passing the debug flag and release mode when passing the release flag? The point is that it doesn't really matter how build scripts are built, just needs to run on the machine it's built on.

@thomcc
Copy link
Member

thomcc commented May 25, 2023

I haven't looked into the issues revealed after landing this deeply, but I think we should probably filter out irrational SDKROOTs.

They'll be wrong when running cc as a dependency of a build script (for example, if compiling for iOS, if you have a build script which wants to use lzma-sys or something to compress some data, then it will need to run cc and compile targeting the host).

We already do this for some environment variables.

@r-value
Copy link

r-value commented Aug 9, 2023

Maybe SDKROOT is just wrong in some situation when cross building?

No, it shouldn't be wrong when compiling for the target. All the links you mention are saying that when compiling native parts, target env variables weren't sanitized correctly.

Do build scripts compile in debug mode when passing the debug flag and release mode when passing the release flag? The point is that it doesn't really matter how build scripts are built, just needs to run on the machine it's built on.

The main issue is that this PR breaks the cross build CI of rustc. Any better proposals? I think we should sanitize SDKROOT anyway.

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.

cc-rs ignores SDKROOT supported by rustc/clang
6 participants