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

Why are OpenMP flags and directories passed to external projects? #502

Closed
paskino opened this issue Mar 1, 2021 · 3 comments · Fixed by #879
Closed

Why are OpenMP flags and directories passed to external projects? #502

paskino opened this issue Mar 1, 2021 · 3 comments · Fixed by #879
Assignees
Labels
Milestone

Comments

@paskino
Copy link
Contributor

paskino commented Mar 1, 2021

I am using the intel compiler and I cannot build for instance parallelproj with OpenMP because we are passing somehow the flags and libraries found to external project via these lines

mark_as_superbuild(ALL_PROJECTS VARS 
      OpenMP_CXX_FLAGS:STRING OpenMP_CXX_LIB_NAMES:STRING OpenMP_C_FLAGS:STRING 
      OpenMP_C_LIB_NAMES:STRING OpenMP_CXX_FLAGS:STRING OpenMP_CXX_LIB_NAMES:STRING 
      OpenMP_libomp_LIBRARY:FILEPATH OpenMP_omp_LIBRARY:FILEPATH
      OpenMP_gomp_LIBRARY:FILEPATH OpenMP_pthread_LIBRARY:FILEPATH
      OPENMP_INCLUDES:PATH OPENMP_LIBRARIES:PATH
      )

This seems to prevent the project's own find_package(OpenMP) to set the variable it requires, probably because find_package looks in the cache before executing. I get the a message that says that the OpenMP_iomp_LIBRARY variable isn't set and the build of the external project fails at cmake configure time.

The solution is to add OpenMP_iomp_LIBRARY to the variables in mark_as_superbuild. However, OpenMP_iomp_LIBRARY is a variable that has meaning only if one tries to build with the intel compiler, while the others seem to be applying to GCC. So, while adding the variable solves the issue, I don't think we should maintain such a list at all.

Actually, removing the above code in mark_as_advanced mark_as_superbuild makes it work as well.

So the question is why do we pass such variables at all?

@paskino paskino changed the title Why are OpenMP flags and directories are passed to external projects? Why are OpenMP flags and directories passed to external projects? Mar 1, 2021
@KrisThielemans
Copy link
Member

@rijobro had to add the capability to pass explicit OpenMP flags, as find_package(OpenMP) didn't work on his OSX,brew,AppleClang or whatever combo. Hence, we have lines like

-DOPENMP_INCLUDES:PATH=${OPENMP_INCLUDES}
-DOPENMP_LIBRARIES:PATH=${OPENMP_LIBRARIES}

Passing these variables isn't necessary on Linux nor Windows as far as I know. However, disabling that functionality seems like a bad idea.

This seems independent however to adding variables to mark_as_superbuild. This is a macro that then automatically adds the related variables to the CMAKE_ARGS of all dependent projects. We originally used this strategy to make our life easier (i.e. avoid doing it by hand). Not sure if that was a good idea...

Actually, removing the above code in mark_as_advanced makes it work as well.

I'm guessing that was a typo, and you meant mark_as_superbuild.

I'm all in favour of removing the above mark_as_superbuild statement. It introduces specific variables, which apparently create trouble. It is possible that we'll need to add OpenMP_CXX_FLAGS in the dependenct projects, but I hope not. As we currently don't run MacOS tests at all, we will only find out when somebody tells us.

So, I suggest to do the following:

  • remove the mark_as_superbuild lines you quote
  • create a variable OpenMP_CMAKE_ARGS setting OPENMP_INCLUDES and OPENMP_LIBRARIES
  • use that variable in all dependent projects where we currently have OpenMP flags, just like we do with Python.

@KrisThielemans KrisThielemans added this to the v2.3 milestone Mar 11, 2021
@paskino
Copy link
Contributor Author

paskino commented Mar 11, 2021

see also #95

@KrisThielemans
Copy link
Member

I'm all in favour of removing the above mark_as_superbuild statement. It introduces specific variables, which apparently create trouble.

Actually, I take this comment back. mark_as_superbuild is equivalent to us creating a variable and passing that, but it's easier, as we don't have to know which dependencies need OpenMP.

So, it's better to pass the iomp library

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants