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

Revert #110 to fix iOS cross-compilation after #158. Fixes #173 #176

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

luser
Copy link
Contributor

@luser luser commented Jan 27, 2023

In #158, better support for using CMake's cross-compilation facilities was added. This made the workaround added in #110 for iOS not only unnecessary, but actively harmful, in that it runs afoul of SDK validation checks in the CMake iOS codepath.

This PR removes the workaround (reverting #110) and also adds a CI job to test an iOS cross-compile.

…Fixes rust-lang#173

In rust-lang#158, better support for using CMake's cross-compilation facilities was
added. This made the workaround added in rust-lang#110 for iOS not only unnecessary,
but actively harmful, in that it runs afoul of SDK validation checks in the
CMake iOS codepath.
@luser luser changed the title add an iOS cross-compile test to CI Revert #110 to fix iOS cross-compilation after #158. Fixes #173 Jan 27, 2023
@luser
Copy link
Contributor Author

luser commented Jan 27, 2023

I pushed the CI change without the revert, and it failed which was good because it reproduced the problem:
https://github.com/luser/cmake-rs/actions/runs/4025276950/jobs/6918367203

@luser
Copy link
Contributor Author

luser commented Jan 27, 2023

The iOS cross-compile job succeeds in CI now.

.github/workflows/main.yml Outdated Show resolved Hide resolved
Comment on lines +98 to +101
- uses: actions-rs/toolchain@v1
with:
toolchain: stable
target: ${{ matrix.platform.target }}
Copy link
Member

Choose a reason for hiding this comment

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

Do not use actions-rs as it's currently unmaintained and has some deprecation warnings that mean the workflow won't work at some point. In this case just invoking rustup with run should be sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it seems this was brought by the current config, it's fine then. I'm going to fix it later along with others, so feel free to leave them as-is!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I just copy/pasted from the existing config for consistency. It's a separate job because I couldn't tell if the cross tool would work on macOS from a brief look at it.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

CI config LGTM!

Comment on lines +106 to +107
# If this isn't specified the default is iOS 7, for which zlib-ng will not compile due to the lack of thread-local storage.
IPHONEOS_DEPLOYMENT_TARGET: 16
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: does this mean iOS 16 would be the minimum supported version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this compilation, yes. I didn't feel like actually trying to figure out what the minimum version of iOS that zlib-ng could build for was, so I just used iOS 16 since that's the current SDK. This won't affect any users of the crate, it's just for compiling zlib-sys in CI here.

Co-authored-by: Yuki Okushi <jtitor@2k36.org>
@thomcc thomcc merged commit 9e1614b into rust-lang:master Mar 23, 2023
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.

3 participants