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

Improve MacOS Compatibility #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

keevindoherty
Copy link

This PR makes the persistence filter code compatible with the MacOS.

Specifically, I added instructions for getting dependencies and compiling on MacOS and additionally made the following modifications:

  • Specified -std=c++11 for all non-GNU CXX compiles (Apple uses AppleClang).
  • Correctly link against libgsl when it is installed via brew on Apple machines. This is protected by an if (APPLE) condition in CMakeLists.txt so it should have no compatibility issues.
  • Link against Python 3.9 and Boost for MacOS machines. Note that this requires specifying the version number of boost-python3 in the environment variable BOOST_LIBRARYDIR (see README.md). This also requires specifying the version of (and path to headers for) your Python installation.

There was also apparently a syntax error with a lambda capture in persistence_filter_test.cc (see the diff). This could have been some kind of library or compiler versioning issue, though. I'm not certain.

Caveats:

  1. Specific version numbers are pinned and not retrieved from the system (to be honest, I'm not sure in all cases above how to do this). Despite this, I've tried to include information in README.md about what to change for MacOS users who may have different versions of e.g. Python, Boost, etc, which I hope would be useful to a user needing to modify this information.
  2. I haven't tested the modified version on a Linux machine, so I don't know for certain if I've made any breaking changes. I might recommend just pulling the latest off my fork and trying to compile on a Linux machine quickly if you have a development environment where this is possible?

@david-m-rosen
Copy link
Owner

Hey d00d, apologies for the long delay in getting back to you on this -- was pretty busy trying to push out the latest updates to the SE-Sync code 😬.

In the course of looking over this PR, I realized that my initial take on the CMakeLists file was ... pretty bad 🤣. (In my defense, I've gotten a lot better at using CMake since I first attempted this 🤣.)

I've rewritten portions of the CMakeLists to search for Boost and Python in a simpler and more portable way (that might hopefully improve interoperability with MacOS?), but it looks like this has introduced some conflicts in the PR. Would you mind trying to merge master on your end and updating the PR? Hopefully these will make building under MacOS easier as well ...

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.

2 participants