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

docs: Recommend macOS 10.11 deployment target #6000

Merged
merged 1 commit into from
Nov 15, 2019

Conversation

theopolis
Copy link
Member

This is an attempt to maintain backwards compatibility for macOS. We should not add this directly to CMake due to recommendations in the CMake documentation, but we can test older deployment targets and recommend it in documentation.

The most significant effect this has for compatibility is on compile-time linking.

This addresses #5993.

@Smjert
Copy link
Member

Smjert commented Nov 5, 2019

pre-built libraries are still built with the 10.14 SDK, moreover it seems that also openssl doesn't properly get this value and compiles with 10.14.

Are we sure that mixing SDK like this isn't an issue?

@directionless
Copy link
Member

Are we sure that mixing SDK like this isn't an issue?

I'm not sure. Though I can say that the test build in #5993 seems to work on my El Capital and my Catalina machine.

I like the approach of not making it the default, but adding it to the Azure pipeline.

@alessandrogario
Copy link
Member

alessandrogario commented Nov 5, 2019

Would it be possible to add a check inside the facebook layer in libraries/cmake/facebook/modules/api.cmake that prints a warning in case the SDK version doesn't match the pre-built binaries?

Something like

if(NOT "${CMAKE_OSX_DEPLOYMENT_TARGET}" STREQUAL "10.14")
  message(WARNING "The pre-built libraries in this layer are targeting 10.14. This build is not supported")
endif()

EDIT: I think we will be able to fully close #5993 once we finish migrating all the libraries to the source layer

@theopolis
Copy link
Member Author

moreover it seems that also openssl doesn't properly get this value and compiles with 10.14

@alessandrogario is there a way to address this, it seems like we should be able to forward this CMake option into OpenSSL?

Are we sure that mixing SDK like this isn't an issue?

Generally it seems not-great. In practice (from when I did this a lot in past projects) there was no observable effect (hey, the tests pass!) We should be able to get the Facebook layer out of the way soon enough.

@theopolis theopolis added this to the 4.1.1 milestone Nov 7, 2019
@theopolis
Copy link
Member Author

(hot take) I think we should merge this for 4.1.1. If this requires porting all of the third-party libs on macOS to the source layer then let's attempt it.

@directionless
Copy link
Member

As I see it, without this we immediately hard fail on older macOS.

With this, we're not totally sure. Do we have a good way to quantify that risk? Or a handful of test cases to try?

Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Conclusion in office house 2019-11-12 was that we should merge this.

It fixes compatibility with older macOS releases. And while incurs an unknown risk of possible issues with extensions, initial testing doesn't show that. (and that risk is, perhaps, best addressed by moving everything to the source layer)

@theopolis theopolis merged commit c8cd366 into osquery:master Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants