Bug 223678 - devel/cmake: find_package(OpenMP) doesn't find the openmp package (when base clang compiler is used)
Summary: devel/cmake: find_package(OpenMP) doesn't find the openmp package (when base ...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-kde (Team)
: 234050 (view as bug list)
Depends on:
Reported: 2017-11-15 07:34 UTC by Yuri Victorovich
Modified: 2019-04-05 15:44 UTC (History)
7 users (show)

See Also:
tcberner: maintainer-feedback+

Replacement for FindOpenMP (22.64 KB, text/plain)
2019-03-10 13:04 UTC, Adriaan de Groot
no flags Details
Diff from installed FindOpenMP (2.33 KB, patch)
2019-03-10 13:04 UTC, Adriaan de Groot
no flags Details | Diff
Replacement for FindIOpenMP (23.86 KB, text/plain)
2019-03-10 23:07 UTC, Adriaan de Groot
no flags Details
Diff from installed FindOpenMP (3.93 KB, patch)
2019-03-10 23:07 UTC, Adriaan de Groot
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuri Victorovich freebsd_committer 2017-11-15 07:34:24 UTC
I put 'find_package(OpenMP)' into CMakeLists.txt, run cmake, and it always prints:
> -- Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES) (found version "1.0")
> -- Could NOT find OpenMP_CXX (missing: OpenMP_CXX_FLAGS OpenMP_CXX_LIB_NAMES) (found version "1.0")

Specifying flags:
> cmake -DCMAKE_C_FLAGS:STRING="-I/usr/local/include" -DCMAKE_EXE_LINKER_FLAGS:STRING="-L/usr/local/lib" CMakeLists.txt
produces the same result.

It worked before devel/cmake-modules was moved into cmake.
Comment 1 Yuri Victorovich freebsd_committer 2017-11-15 07:43:09 UTC
Looking again at /usr/local/share/cmake/Modules/FindOpenMP.cmake, it seems broken. OpenMP_C_CXX_TEST_SOURCE is now defined and not used. It was used when devel/cmake-modules was a separate port.
Comment 2 Yuri Victorovich freebsd_committer 2017-11-15 08:22:44 UTC

It looks like find_package(OpenMP) got broken by the merge of devel/cmake-modules into cmake, commit r449853.

Could you please take a look?

Thank you!
Comment 3 Adriaan de Groot freebsd_committer 2017-11-15 23:02:26 UTC
*did* it actually work before? Because a simple `find_package(OpenMP)` doesn't find it on my 12-current system, either with CMake 3.8.2 (before the merge) or CMake 3.9 (after). The module itself was changed heavily upstream between those two releases, so it's probably not so much the merge as the update itself.

Have you got a port / sample CMakeLists that actually needs OpenMP (and now fails to find stuff)?
Comment 4 Yuri Victorovich freebsd_committer 2017-11-16 04:58:25 UTC
(In reply to Adriaan de Groot from comment #3)

Standalone port worked, but I had to set CMAKE_REQUIRED_LIBRARIES for check_c_source_compiles that was there. Now FindOpenMP.cmake doesn't have check_c_source_compiles. Setting CMAKE_REQUIRED_LIBRARIES was also wrong.

So it was broken before and now it is still broken in somewhat different way. Merge probably has nothing to do with this.

I created the upstream bug report: https://gitlab.kitware.com/cmake/cmake/issues/17474
Comment 5 Adriaan de Groot freebsd_committer 2017-11-16 13:23:00 UTC
Does base clang support OpenMP (ever?). I just tried on a 10.4 box; complete logs are in the upstream bug report, but basically:

 - using base clang doesn't find OpenMP
 - using clang40 (e.g. setting CC in the environment) does find OpenMP
Comment 6 Adriaan de Groot freebsd_committer 2017-11-16 14:53:12 UTC
On 12-CURRENT amd64, base clang will find OpenMP if I add `-DCMAKE_CXX_FLAGS="-I/usr/local/include -L/usr/local/lib"` (which makes sense, given that OpenMP is found in $LOCALBASE). So it may be sufficient to massage the Find-module to look in more places when searching for includes and libraries.
Comment 7 Christian Pfeiffer 2017-11-16 15:31:55 UTC
(In reply to Adriaan de Groot from comment #6)

Does clang -fopenmp itself work, or do you also need to pass both $LOCALBASE as include and link path for that flag to work at all?

FindOpenMP will not attempt anything beyond invoking the compiler with the given CMAKE_<LANG>_FLAGS and the flags to try out, e.g. -fopenmp. If you install the OpenMP headers / libraries in a different location than the compiler's default search paths, any attempt to compile a source file will fail due to the lack of OpenMP library and/or header.

Note that FindOpenMP currently has no OpenMP_<LANG>_INCLUDE_DIR variable due to this assumption.
Comment 8 Adriaan de Groot freebsd_committer 2017-11-16 23:25:47 UTC
@christian, clang -fopenmp itself works, e.g.:

    $ echo "int main() { return 0; }" > t.c
    $ cc -fopenmp -c t.c

To know if the flag itself works, `-fopenmp -Werror` could be used (assuming clang), without needing to `#include <omp.h>`. That would weed out base clang on 10.x.

Still, this looks like a case for doing a patch in the packaging, where we can `try_compile()` a source file that **just** does the include, then possibly use `find_file()`, to pin down where omp.h actually lives.

There's additional unfortunate variation between the various machines I have and where OpenMP lives on them. I think the right thing to do is patch it here, and leave upstream FindOpenMP alone.
Comment 9 Christian Pfeiffer 2017-11-16 23:53:06 UTC
>To know if the flag itself works, `-fopenmp -Werror` could be used (assuming clang), without needing to `#include <omp.h>`. That would weed out base clang on 10.x.

`OpenMP_FOUND` should however imply that the `omp.h` file is available for include since that's part of the OpenMP standard.

>Still, this looks like a case for doing a patch in the packaging, where we can `try_compile()` a source file that **just** does the include, then possibly use `find_file()`, to pin down where omp.h actually lives.

CMake tries to work in any environment without specific patches needed. If FreeBSD needs $LOCALBASE as a hint to the include and library paths then that's considered a CMake defect for that reason.

Adding support for additional library hints and an include folder isn't a problem at all, I just didn't realize the problem so far because nobody had reported it.

>There's additional unfortunate variation between the various machines I have and where OpenMP lives on them.

My understanding would be it should be $LOCALBASE though, right? No matter how this gets resolved, a way to determine which OpenMP include and library paths are required would be needed to generate usable OpenMP settings.
Comment 10 Adriaan de Groot freebsd_committer 2017-11-17 09:58:54 UTC
I've looked more closely at the `devel/openmp` port:
 - it installs ${LOCALBASE}/include/omp.h
 - it installs ${LOCALBASE}/lib/libomp.so

So doing a find_file and find_library with /usr/local and $ENV{LOCALBASE}, then adding the paths to include- and link-flags seems like the best thing to do. I'm personally a bit cagey about adding them because there are cases where you **don't** want /usr/local/include in the search path (or you want it in a specific place in the search order), but let's cross that bridge when we come to it.

@christian, you want to add that and shoot us some patches, or should we develop it and submit upstream? In theory my NAS is building nightlies of cmake and submitting them to cdash (cmake.bionicmutton.org) so you can get automatic feedback that way.
Comment 11 Christian Pfeiffer 2017-11-17 12:13:53 UTC
Okay, that was my understanding of it as well. I couldn't check myself yesterday since I didn't have my FreeBSD box at hand, but I do now.

I think the solution would be to try the current compiles first and if they all fail, retry them with the $LOCALBASE paths added to them. That's roughly what I did for supporting FreeBSD's MPI libraries in the ports for FindMPI in 3.10.

This way, if a user has built their own Clang / GCC, the $LOCALBASE/include/omp.h wouldn't take precedence over the compiler provided headers and would still be found for base Clang. Of course, this means putting the whole $LOCALBASE/include in the include path, but this can hardly be avoided since the use of any OpenMP support routine requires using that header. Since it's part of the OpenMP standard, I don't think that making it optional or depending on a CMake component for FindOpenMP would be a good idea, either. The library won't be a problem though, since `find_library` determines the full path and there won't be any `-L` flag being used.

@adriaan I don't mind doing it at all. Since 3.10 is due to release very shortly, it won't be making it into that release, though. I would suggest that I develop an upstream patch with the outlined methodology for 3.11 and we can then see about getting it into a 3.10 point release or applying that patch downstream until the next release comes out.
Comment 12 Christian Pfeiffer 2017-11-17 15:48:59 UTC
@adriaan I had a look through the cmake ports description and noticed you generally replace `/usr/local` with $LOCALBASE using sed and applied some patches.

From what I explained above, it's in both CMake's and FreeBSD's best interest to keep the number of patches and adaptions needed as small as somehow possible. For one, upstream does nightly testing with *unpatched* CMake trunks, and that includes FreeBSD test machines. Secondly, some packages require FreeBSD specific adaptations going beyond just replacing `/usr/local` with $LOCALBASE, e.g. FindMPI needs to consider all subdirectories for `/usr/local/mpi` and that's something just introduced with 3.10 now.

I think the `/usr/local` hard coding is an issue that needs addressing in CMake itself instead of patching it downstream. One of the reasons is that by simply replacing `/usr/local` using sed, you'll get strange results for cross-compilation builds using a `CMAKE_SYSROOT` where it should be `/usr/local`. For OpenBSD this issue is as a matter of fact addressed upstream, see here: https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/Platform/OpenBSD.cmake#L28-37 The exact same approach could be used for FreeBSD.

Given the other path replacements, I think the approach used in the OpenBSD platform file would address all of these, but it needs a bit of testing. I can cook up a patch upstream and we can see if that already works out?

Regarding the patches: The wxWidgets, wxWindows and ImageMagick ones could be merged upstream as is.

The FindSDL one should be done with care, if you install SDL manually in an isolated prefix environment, system CMake would then always include the system path - this breaks cross compilation environments for instance. The superior choice would be to use `find_path` to determine the header path in order to not break 

I'm not quite certain what the FindQt4 one is trying to achieve, but it will overwrite user-set variables and also break cross compilation scenarios due to `CMAKE_SYSROOT` not being searched anymore. If there's a scenario where this is needed, that sounds like a bug in the module.

In general, given CMake's set goals, you should consider any downstream bug for CMake an upstream bug as well. FreeBSD is a platform that CMake strives to support fully and needed downstream adjustments are therefore a bug.
Comment 13 Adriaan de Groot freebsd_committer 2018-04-07 22:35:06 UTC
For CMake 3.11, a new issue is introduced where `find_package(OpenMP)` does the right thing and finds -fopenmp as the right flag, and then it goes looking for supporting libraries. With CMake 3.11, NO_ROOT_PATH (?) is used, and that prevents it from finding `-lpthread`.

I'm going to solve that with a hammer in the FreeBSD packaging with

set(OpenMP_pthread_LIBRARY -lpthread)

near the top of the module. That stops it from being searched for.
Comment 14 commit-hook freebsd_committer 2018-04-15 21:44:06 UTC
A commit references this bug:

Author: adridg
Date: Sun Apr 15 21:43:58 UTC 2018
New revision: 467437
URL: https://svnweb.freebsd.org/changeset/ports/467437

  Update CMake to 3.11.0. Thanks to antoine@ for the exp-run.

  In the run-up to this commit, many other ports were pre-emptively fixed.
  The only issue still known is math/kig, which had a build failure in
  the exp-run, but which isn't reproducible across multiple 11.1 {i386,amd64}
  machines and poudriere builds. We've decided to forge ahead.

  The new CMake version:
   - drops FreeBSD patches that have been incorporated upstream,
   - re-shuffles patches to FindQt4, since upstream has made some changes
     which break FindQt4 in new ways on FreeBSD (while fixing the old ones),
   - has new patches to make OpenMP and BLAS findable on FreeBSD,
   - drops ports libarchive in favor of the version in base, to avoid
     overlinking for the pkg(8) support in CPack (this makes portlint
     complain, and we have decided to ignore it).

  PR:		227372 226959 223678
  Approved by:	tcberner (mentor)
  Differential Revision:	https://reviews.freebsd.org/D14506

Comment 15 Adriaan de Groot freebsd_committer 2018-04-18 06:54:32 UTC
Plain find_package(OpenMP) now works, as do ports that use it (e.g. Wesnoth).
Comment 16 Yuri Victorovich freebsd_committer 2018-08-31 07:53:21 UTC
For some reason, find_package(OpenMP) still doesn't work:
-- Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES) 
-- Could NOT find OpenMP_CXX (missing: OpenMP_CXX_FLAGS OpenMP_CXX_LIB_NAMES) 
-- Could NOT find OpenMP (missing: OpenMP_C_FOUND OpenMP_CXX_FOUND) 

cmake-3.12.1 and openmp-6.0.1_1 are installed on 11.2
Comment 17 Yuri Victorovich freebsd_committer 2018-08-31 16:10:09 UTC
Reopening: the problem exists on 11.2 and CURRENT.
Comment 18 Yuri Victorovich freebsd_committer 2018-12-16 08:28:50 UTC
See also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=234050
Comment 19 Adriaan de Groot freebsd_committer 2018-12-24 14:01:31 UTC
Both PR 234050 and PR 223678 need someone to chase what CMake is doing exactly -- you may find the cmake flags --debug-trycompile and --trace useful -- and to propose a patch. This isn't something we have time for right now.
Comment 20 Adriaan de Groot freebsd_committer 2019-03-10 13:04:12 UTC
Created attachment 202773 [details]
Replacement for FindOpenMP
Comment 21 Adriaan de Groot freebsd_committer 2019-03-10 13:04:50 UTC
Created attachment 202774 [details]
Diff from installed FindOpenMP
Comment 22 Adriaan de Groot freebsd_committer 2019-03-10 13:20:19 UTC
Yuri, here's a patched-up FindOpenMP.cmake (and the diff between the one included with cmake-3.13.3 and the patch-up version). It basically hits things with a hammer again until stuff works.

 - When checking for OpenMP support, we still need to add the right include paths for <omp.h>, and then linking paths for libopenmp (assuming devel/openmp is installed, not the one from an llvm port).
 - If a port Makefile messes around with CXXFLAGS and LDFLAGS (e.g. adding LOCALBASE) then this would work, but for a "bare" compile, those directories are not searched.

So this patch does:
 - Hammer the necessary -I and -L flags into try_compile()
 - Hammer them into the exported link target ("Modern" style). This causes warnings about -L flags in compile lines, though.

It gets your test-case from PR 234050 to build and run in a 12.0-R VM, with no extra environment manipulation. It is, however, rather ugly (especially the way it adds flags to the exported targets). Can you test this against the bits you have? Just replace /usr/local/share/cmake/Modules/FindOpenMP.cmake .
Comment 23 Adriaan de Groot freebsd_committer 2019-03-10 22:44:25 UTC
*** Bug 234050 has been marked as a duplicate of this bug. ***
Comment 24 Adriaan de Groot freebsd_committer 2019-03-10 22:52:48 UTC
A couple of things need(ed) doing:
 - -fopenmp also needs to be passed as a link flag for all variants
 - -L needs to be passed to base clang
 - -I needs to be passed to base clang

With a non-base clang, CMake will report
-- Found OpenMP_C: -fopenmp=libomp (found version "3.1")

With base clang and devel/openmp, CMake will report:
-- Found OpenMP_C: -fopenmp=libomp -I/usr/local/include -L/usr/local/include (found version "3.1")

Suitable subsets of those flags are set on the exported target for "Modern" usage. I'll update the attachements (module and diff) in a few minutes.
Comment 25 Adriaan de Groot freebsd_committer 2019-03-10 23:07:21 UTC
Created attachment 202785 [details]
Replacement for FindIOpenMP
Comment 26 Adriaan de Groot freebsd_committer 2019-03-10 23:07:55 UTC
Created attachment 202786 [details]
Diff from installed FindOpenMP
Comment 27 Adriaan de Groot freebsd_committer 2019-03-10 23:20:42 UTC
@yuri in particular: please replace /usr/local/share/cmake/Modules/FindOpenMP.cmake with the one attached (or apply the diff) and do your testing. These changes aren't really upstreamable, but could go in with the CMake 3.14 exp-run.
Comment 28 Yuri Victorovich freebsd_committer 2019-03-11 04:41:30 UTC
(In reply to Adriaan de Groot from comment #27)

Hi Adriaan,

I tried several ports that were breaking because of OpenMP. With this patch they detect OpenMP okay now.

Thank you for fixing this!
Comment 29 commit-hook freebsd_committer 2019-04-05 11:46:59 UTC
A commit references this bug:

Author: adridg
Date: Fri Apr  5 11:46:46 UTC 2019
New revision: 497948
URL: https://svnweb.freebsd.org/changeset/ports/497948

  Update CMake to latest release, 3.14.1

  Changelog: https://cmake.org/cmake/help/v3.14/release/3.14.html
  Local patches: fixes for Boost, Python, and OpenMP

  Affected ports:
   - opencpn, hugin needed help in finding wx
   - kadu is over-enthusiastic in finding non-existent X11 components
   - xlife++ does weird things in parsing help output from cmake

  Thanks antoine@ for multiple exp-runs.
  Thanks tcberner@ for much prep-work.

  PR:		236534 223678 227428