Add logic to detect libomp installs in homebrew and macports on macOS

[rayg-ssec]

When building using Clang, libomp is the preferred OpenMP library, though it's a separate build from the main compiler.

macOS defaults to clang (though GCC can be made available), but unless libomp is installed (often but not always through Homebrew or MacPorts), building without OMP is the baseline.

In a manual install of libomp, you need slightly different compile flags -Xpreprocessor -fopenmp and appropriate linkage to libomp, with include and library search paths set in CFLAGS and LDFLAGS environment variables.

This patch attempts to retain backward compatible behavior, but extend USE_OMP beyond 1/0 to "clang" or "omp", "gcc" or "gomp" to be more specific with respect to compiler options.

It also, on macOS, attempts to detect libomp if it is installed via brew or port by listing the manifest of the installed packages, and generating CFLAGS and LDFLAGS that would have been needed to satisfy -Xpreprocessor -fopenmp and -lomp compiler/linker directives. If USE_OMP is not specified, it will probe to see if USE_OMP=omp can be inferred by checking for libomp include and library paths.

This means pip install . on macOS systems with libomp available should succeed with OpenMP enabled by default, while permitting non-macOS systems using clang to set CFLAGS/LDFLAGS and USE_OMP=omp . USE_OMP=0 should also still be obeyed. A brief debug line is included when -v is enabled on the pip install.

[djhoese]

This is adding so much needed functionality. Thanks for starting down this path and trying to combine all the various ways this can go. I've made a lot of comments about other possibilities for parts of this. Let me know what you think.

[djhoese]

This is really looking clean. I have a couple requests, but the majority of the functionality makes sense to me. I'm just hoping it can be cleaned up a bit more for future flexibility (if needed) while also avoiding running things at import/load time.

[djhoese]

More questions and comments. Getting really close though.

[djhoese]

Besides the print statement use_omp isn't used anymore, right? It makes me wonder if those if statements in the above helper method can be simplified even further. Perhaps getting OMP_COMPILE_ARGS.get(use_omp, []) earlier in the method and then adding macos_compile_args later on if needed...hhmmm except you might force omp later. Not sure.

The print could maybe be moved up into the helper or removed? Thoughts?

[rayg-ssec]

The print only shows up if you add the -v to the pip, currently; if there's a better way to have an "info" message during a setup.py, that would be preferred. Having it as optional debug is useful, to show concisely what choices have been taken. Placing it inside the _omp_compile_link_args would require a refactoring to avoid a DRY violation. Might be straying a bit too far into the white glove treatment, in other words.

I have more concerns about making sure Windows builds still work, and edge cases like "I have a linux distro that's clang-focused" (OpenMandriva?) have a path to build without having to edit the setup.py. I think that's the case, given I can swap between gcc/gomp and clang/omp on macos successfully by just changing environment values.

[djhoese]

Placing it inside the _omp_compile_link_args would require a refactoring to avoid a DRY violation.

Why? Because there are two return statements? My instinct is to think any refactoring down those lines would only make the code cleaner. For example, right now you set use_omp = None in an else statement, but that has no usage or meaning anywhere else in the code. If you wrapped the mac stuff into a new function or had macOS_omp_options_from_probe return empty lists instead of None then the rest of the function/if statements just goes away. Or at least you could do something like:

compile_args = []
link_args = []
...

macos_compile_args, macos_link_args = macOS_omp_options_from_probe()
if macos_compile_args and macos_link_args:
    compile_args += macos_compile_args
    link_args += macos_link_args

...

Hm maybe this doesn't clean it up this much. I completely didn't include the OMP_COMPILE_ARGS. I think there is a slightly cleaner way to structure it, but maybe it isn't worth the effort.

[djhoese]

Assuming MSVC can handle loading openmp by default as you have it, what if it was split up like this:

if use_omp == "probe":
    if is_macOS() and not is_conda_interpreter():
        use_omp = "omp"
    elif compiler == "msvc":
        use_omp = "msvc"
    else:
        use_omp = "gomp"
    print(f"Probing OpenMP support will attempt to use {use_omp}")
compile_args = []
link_args = []
if use_omp == "omp" and is_macOS():
    extra_compile_args, extra_link_args = macOS_omp_options_from_probe()
    if not extra_compile_args:
        print("Could not find OpenMP on MacOS")
    compile_args += extra_compile_args
    link_args += extra_link_args
compile_args += OMP_COMPILE_ARGS.get(use_omp, [])
link_args += OMP_LINK_ARGS.get(use_comp, [])

Put in to two (or three) separate helper functions this clears up "guess OpenMP usage based on platform/compiler" and "collect all compile and link args for OpenMP".

[djhoese]

Placing it inside the _omp_compile_link_args would require a refactoring to avoid a DRY violation.

Why? Because there are two return statements? My instinct is to think any refactoring down those lines would only make the code cleaner. For example, right now you set use_omp = None in an else statement, but that has no usage or meaning anywhere else in the code. If you wrapped the mac stuff into a new function or had macOS_omp_options_from_probe return empty lists instead of None then the rest of the function/if statements just goes away. Or at least you could do something like:

compile_args = []
link_args = []
...

macos_compile_args, macos_link_args = macOS_omp_options_from_probe()
if macos_compile_args and macos_link_args:
    compile_args += macos_compile_args
    link_args += macos_link_args

...

Hm maybe this doesn't clean it up this much. I completely didn't include the OMP_COMPILE_ARGS. I think there is a slightly cleaner way to structure it, but maybe it isn't worth the effort.

[rayg-ssec]

The most recent commits refactor it down to a lot of what you've recommended. I also addressed the "I know what I'm doing and don't want args probed" expert user case - brew and ports checks are only done in probe mode.

[djhoese]

Did you see my pull request here: rayg-ssec#1

[rayg-ssec]

Did you see my pull request here: rayg-ssec#1

No, ironically I decided to add the GitHub app to my phone and tablet today in hopes of getting better notifications and work queue. Didn't see the PR until you pointed it out.

[rayg-ssec]

The merge of your changes ran with no issue through my 5 local test cases. The README update is much appreciated.

(I still think reads-like-a-novel is easier to follow than reads-like-a-newspaper however!)

[djhoese]

Amazing! I think this is as good as we can get this without getting more user feedback. I'll merge this and then try to do a patch release. Then...we'll wait for complaints.