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

Fix building for iOS #767

Merged
merged 3 commits into from
Aug 21, 2021
Merged

Conversation

keith
Copy link
Member

@keith keith commented Aug 19, 2021

Some projects like envoy-mobile use rules_foreign_cc to build targeting
iOS, so this is useful to validate on CI as part of the macOS test
build. This also fixes a bug where this was broken since
c41020e because the wrong SDKROOT was used

examples/WORKSPACE.bazel Outdated Show resolved Hide resolved
@keith
Copy link
Member Author

keith commented Aug 19, 2021

So I currently expect this to fail, since compatibility with iOS was broken somewhere in this diff d54c78a...b51f25e which we discovered when updating envoy and envoy-mobile's usage of the rules.

My assumption is that it was caused by c41020e which always assumes SDKROOT is macOS specific. But I haven't been able to fix it yet in my tests even after fixing that. The -isysroot arg cmake produces is always pointing to a macOS SDK

@keith
Copy link
Member Author

keith commented Aug 19, 2021

cc @jsharpe from the commit above hoping you have some ideas here

@keith
Copy link
Member Author

keith commented Aug 19, 2021

@keith keith force-pushed the ks/add-ios-test-case branch from 8ea64a8 to ba50ced Compare August 19, 2021 22:29
@keith keith changed the title Add iOS test case Fix building for iOS Aug 20, 2021
@keith
Copy link
Member Author

keith commented Aug 20, 2021

Ok confirmed that was the issue and pushed a fix here. I was thrown off while trying to reduce because of #769

@keith keith marked this pull request as ready for review August 20, 2021 02:06
@keith keith force-pushed the ks/add-ios-test-case branch 2 times, most recently from a90b8b6 to 0ac298b Compare August 20, 2021 05:53
@keith
Copy link
Member Author

keith commented Aug 20, 2021

I don't see how this could have affected the Linux python3 setup, but I will try to debug more

@jsharpe
Copy link
Member

jsharpe commented Aug 20, 2021

I don't see how this could have affected the Linux python3 setup, but I will try to debug more

Yes, that's an odd failure; I can't see how this change would have affected the RBE python3 build either.

keith added 2 commits August 20, 2021 15:29
Some projects like envoy-mobile use rules_foreign_cc to build targeting
iOS, so this is useful to validate on CI as part of the macOS test
build.
@keith keith force-pushed the ks/add-ios-test-case branch from 0ac298b to db9b684 Compare August 20, 2021 22:30
@keith
Copy link
Member Author

keith commented Aug 20, 2021

My current assumption is that by changing the workspaces here I'm triggering a build that was previously cached, and somehow rotted, maybe through a docker image update

@keith keith force-pushed the ks/add-ios-test-case branch from 62b4a25 to dcae5f5 Compare August 20, 2021 23:23
@keith
Copy link
Member Author

keith commented Aug 20, 2021

Ok I take it back, it seems to be a workspace ordering issue. I think because of some transitive dependency on zlib

@keith keith requested review from jsharpe and UebelAndre August 20, 2021 23:32
Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

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

Thanks for getting to the bottom of this! It's very useful to have more diverse use cases in the tests

@jsharpe jsharpe merged commit da8952e into bazel-contrib:main Aug 21, 2021
@keith keith deleted the ks/add-ios-test-case branch August 21, 2021 19:18
@keith
Copy link
Member Author

keith commented Aug 21, 2021

❤️ thanks!

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