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

Set Xcode configuration only when building objc #1555

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

siddharthab
Copy link
Contributor

Resolves #1554.

@steeve Can you also take a look and see if this change works for you?

@steeve
Copy link
Contributor

steeve commented Jun 17, 2018

Unfortunately that won't work, since those configurations are needed across all build targets (stdlib even).
However, since I believe the issue is only on MacOS, we can try to disable it from being recognized as an Apple Platform needing configuration. Just remove it from https://github.com/bazelbuild/rules_go/blob/master/go/platform/apple.bzl#L26 and that may solve your problem.

That will prevent from accessing apple_common.XcodeVersionConfig.

@siddharthab
Copy link
Contributor Author

Are you suggesting I disable it behind a feature flag? I.e. a flag on the command line stops the function from being called.

@steeve
Copy link
Contributor

steeve commented Jun 17, 2018

I meant try to comment out the line I mentioned, it will then ignore mac OS as an Apple platform.

@siddharthab
Copy link
Contributor Author

Oh yes, that definitely works. And so does providing macos_sdk_version flag. I am trying to get a fix merged here so my organisation does not have to maintain its own fork.

@steeve
Copy link
Contributor

steeve commented Jun 17, 2018

Sure. I also mean that removing the line altogether may be a better way to fix that than the PR in its actual form which will break iOS build.

@steeve
Copy link
Contributor

steeve commented Jun 17, 2018

Ie the PR could be removing the line :)

@siddharthab
Copy link
Contributor Author

I see. I updated the PR.

Will having objc code still work for macOS platform, or are we saying we should not support it?

@steeve
Copy link
Contributor

steeve commented Jun 17, 2018

It should still work, although you're right that needs testing.
These env vars are here for cc_library. I think objc_library will work fine, but that needs testing.

@jayconrod
Copy link
Contributor

I'm not sure I fully understand the impact of this change. Will this break builds with C/C++/ObjC code on macOS?

@siddharthab
Copy link
Contributor Author

I am not sure. @steeve says they should work, and the Travis tests pass. Is there some more testing I can do?

@jayconrod
Copy link
Contributor

Travis and BuildKite all seem to be passing, but I doing think we have a CI configuration that tests macOS with only Xcode command line tools; I think all CI machines have Xcode installed.

On my laptop, I tried uninstalling Xcode and reinstalling just the command line tools. Builds that include objc fail with the same message with and without this change. Is that intentional, or is everything supposed to pass after this change?

@siddharthab
Copy link
Contributor Author

siddharthab commented Jun 19, 2018

Yes, I think Objective C definitely needs Xcode. We want to ensure two things with this change:

  1. C/C++ builds continue to work without Xcode for macOS platform.
  2. ObjC builds continue to work with Xcode for macOS platform.

I think that Travis checked for condition 2, and you and I checked for condition 1. So we should be good now. 😄

@jayconrod jayconrod merged commit 07518b2 into bazel-contrib:master Jun 19, 2018
@jayconrod
Copy link
Contributor

I hope so :) I'm not familiar enough with ObjC and Xcode support in Bazel to be confident in this change, but it seems like it's working so...

ArielleA pushed a commit to ArielleA/rules_go that referenced this pull request Jun 19, 2018
@siddharthab siddharthab deleted the objc branch July 1, 2018 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants