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

Restore macOS 10.10 version compatibility to Bazel #11565

Closed
wants to merge 4 commits into from

Conversation

nikhilm
Copy link
Contributor

@nikhilm nikhilm commented Jun 8, 2020

This is part of the fixes for #11442. (Not a complete fix because it does not address the bazel 2 branch).

It makes the macOS deployment target 10.10. Then it wraps the calls to the macOS logging infrastructure so they only execute on macOS 10.12+.

I've tested this change and a similar cherry-pick on tag 2.2.0 and confirmed this results in a functioning Bazel (at least for the targets we build) on macOS 10.10.

I have questions around what I can do better:

  1. I have just added a universal .bazelrc entry. Is there some way to gate this to just macOS in the Bazel infrastructure? Is that even necessary?
  2. The Bazel tagged versions are just tags and not branches, which means I cannot submit a PR for a backport to Bazel 2.2. How would you like to handle this?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@nikhilm
Copy link
Contributor Author

nikhilm commented Jun 8, 2020

@googlebot I signed it! (as nikhilm@dropbox.com)

@nikhilm
Copy link
Contributor Author

nikhilm commented Jun 8, 2020

@googlebot I signed it!

@nikhilm
Copy link
Contributor Author

nikhilm commented Jun 19, 2020

perhaps @tetromino, @aiuto or @philwo could take a look at this?

I don't know why cla/google is still failing. possibly needs manual intervention.

@philwo
Copy link
Member

philwo commented Jun 19, 2020

Thank you and sorry for the delay in reviewing it! It looks good to me.

The CLA bot told me that it's confused because the PR was created by your gmail.com address (although the commits show your Dropbox address) and because it doesn't know that they belong to the same person, it doesn't like it. Just to verify - have you added your dropbox.com e-mail to your GitHub account as a secondary e-mail address? I think it should work then.

@nikhilm
Copy link
Contributor Author

nikhilm commented Jun 19, 2020

ah! that is what was missing. let's try again.

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@nikhilm
Copy link
Contributor Author

nikhilm commented Jun 19, 2020

@philwo what about

I have just added a universal .bazelrc entry. Is there some way to gate this to just macOS in the Bazel infrastructure? Is that even necessary?
The Bazel tagged versions are just tags and not branches, which means I cannot submit a PR for a backport to Bazel 2.2. How would you like to handle this?

@philwo
Copy link
Member

philwo commented Jun 19, 2020

@nikhilm Nice, thanks! :)

Regarding the bazelrc change - if CI passed, it apparently doesn't break the build on Linux and Windows. 😄

OTOH maybe this is a good time to add build --enable_platform_specific_config to our .bazelrc and then change your line to build:macos --macos_minimum_os=10.10 - this would ensure that the flag only gets set when you're building on macOS. Here's the documentation for that flag.

@nikhilm nikhilm changed the title First run at restoring older macOS version compatibility to Bazel Restore macOS 10.10 version compatibility to Bazel Jun 19, 2020
@philwo
Copy link
Member

philwo commented Jun 26, 2020

Sorry for the delay. I'm importing this now.

@philwo
Copy link
Member

philwo commented Jun 26, 2020

I got this compiler error when trying to submit the PR internally:

third_party/bazel/src/main/native/unix_jni_darwin.cc:156:7: error: comparison of function 'os_log_create' not equal to a null pointer is always true [-Werror,-Wtautological-pointer-compare]
  if (os_log_create != NULL) {
      ^~~~~~~~~~~~~    ~~~~
third_party/bazel/src/main/native/unix_jni_darwin.cc:156:7: note: prefix with the address-of operator to silence this warning
  if (os_log_create != NULL) {
      ^
      &
1 error generated.

Any idea how to fix this? Is it just a matter of prefixing os_log_create with &? (I mean, that's what the message says, but I wanted to check with you to make sure this actually does the right thing 😊)

@nikhilm
Copy link
Contributor Author

nikhilm commented Jun 26, 2020

Ah! good catch. I have updated the PR to have &.

@philwo
Copy link
Member

philwo commented Jun 26, 2020

Perfect, thanks! All presubmit tests pass - it's now out for a quick review by a colleague and should then be auto-pushed to GitHub.

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.

3 participants