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

Update macOS build instructions #248

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

Conversation

RalphORama
Copy link

The build instructions for macOS are fairly out of date. This PR updates build docs in the following ways:

  1. Makes instructions clearer.
    • Currently, docs say "Install OpenJDK 17" then provide a homebrew command to install OpenJDK 8 which fails on current macOS systems (no arm candidate)
  2. Splits commands up into more sensible chunks.
    • The current docs specify installing XCode CLI tools after using a homebrew command. XCode CLI tools are installed as part of the homebrew install process.
  3. Provides a better description of configurable CMake options
    • In the current docs, some CMake options are described in # comments which break the newline escaping of the cmake command. I have moved these comments to bullet points above the cmake command.
  4. Adds OpenJDK 17 to the environment
    • When installing openjdk@17 with homebrew, binaries and includes are not added to the environment by default. Instead of adding symlinks we can just use these two export commands for the build step.
  5. Updates cmake flags
    • The current docs specify -DCMAKE_OSX_DEPLOYMENT_TARGET=10.14 which will not compile because std::filesystem::path was introduced in 10.15
    • 10.14 is fairly out of date. I've updated the flag to specify 11.7, but this can be reduced to 10.15 and still compile if the maintainers still want to support Catalina.
  6. Add -S and -B flags
    • I'm not super familiar with CMake and it was throwing warnings saying source/build directories weren't specified. Adding these flags doesn't change how the CMake step works and avoids errors with future versions of CMake
  7. Add -j4 to the make install step.
    • Threading speeds up compile times. I also added a comment explaining what -j4 means.

@LennyMcLennington
Copy link
Member

@RalphORama Hi, thank you for this update to the documentation, it looks good generally, but why do you set CPPFLAGS for preprocessor flags for an include directory that isn't required to build PolyMC? I don't even think CMake respects the CPPFLAGS environment variable, what is it used for? What happens if you don't specify it?

@RalphORama
Copy link
Author

@RalphORama Hi, thank you for this update to the documentation, it looks good generally, but why do you set CPPFLAGS for preprocessor flags for an include directory that isn't required to build PolyMC? I don't even think CMake respects the CPPFLAGS environment variable, what is it used for? What happens if you don't specify it?

I added that step because the current docs specify installing openjdk@17 with homebrew, but doing so does not add openjdk to PATH. I assumed previously that openjdk includes were needed for the build, but if they aren't you could remove that step entirely. I have not tested building without those export commands first.

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