oneDAL icon indicating copy to clipboard operation
oneDAL copied to clipboard

Improvements in cmake configuration

Open emmenlau opened this issue 2 years ago • 1 comments

Describe the bug I've just started using the oneDAL cmake configuration that is generated from oneDALConfig.cmake.in. It works well, thanks a lot for this helpful tool! However I've found a number of small shortcomings. Attached is also a patch, but it was generated from the official Intel oneAPI binaries, so it may or may not apply 100% cleanly to the sources here.

General Issues Sometimes it can happen that cmake may try to find a package repeatedly. This can happen for example because an upstream package is using find_dependency(DAL) and the current package is also trying to find oneDAL: find_package(DAL). However it seems that oneDALConfig.cmake.in is not fit for this. Here are some issues I found:

  • The variable DAL_LIBS is not cleared at the beginning, so it will always become longer and longer.
  • The section if (NOT TARGET oneDAL::${_dal_component}) does not cover all of the foreach()-loop. Therefore if a target already exists, set_target_properties() will be called on the existing target, however the library path _dal_lib is empty on the second run. Therefore all targets have no IMPORTED_LOCATION.

Here a patch that resolves these issues: oneDALConfig.cmake.patch.gz

Windows Debug Build Issues On Windows, the cmake package configuration does currently not respect the debug library names. The debug versions of libraries have a suffix d at the end. But the cmake configuration will try to link the release versions (without the suffix). Here is a small patch with a suggested solution: oneDALConfig.cmake-win32-debug-support.patch.gz

Naming Suggestions Another small issue is the names of the configuration variables. Its great that the cmake configuration can be configured with the help of variables. Currently their names are TARGET_LINK, USE_PARALLEL, USE_DPCPP, and USE_NEW_IFACES. Some good improvements would be:

  • to make these names specific to oneDAL. In MKL they use the prefix MKL_, maybe oneDAL could do the same? For large projects it can otherwise be a bit confusing when users override these settings from outside. I use now -DTARGET_LINK=static in several projects which can confuse other users that my whole project would link static, not just static oneDAL.
  • The names are not very consistent with MKL. Maybe they could be made more consistent? MKL currently uses the following names and values:
# MKL_ARCH
#    Values:  ia32 intel64
#    Default: intel64
# MKL_LINK
#    Values:  static, dynamic, sdl
#    Default: dynamic
#       Exceptions:- DPC++ doesn't support sdl
# MKL_THREADING
#    Values:  sequential,
#             intel_thread (Intel OpenMP),
#             gnu_thread (GNU OpenMP),
#             pgi_thread (PGI OpenMP),
#             tbb_thread
#    Default: intel_thread
#       Exceptions:- DPC++ defaults to tbb, PGI compiler on Windows defaults to pgi_thread
# MKL_INTERFACE (for MKL_ARCH=intel64 only)
#    Values:  lp64, ilp64
#       GNU or INTEL interface will be selected based on Compiler.
#    Default: ilp64
# MKL_MPI
#    Values:  intelmpi, mpich, openmpi, msmpi, mshpc
#    Default: intelmpi

emmenlau avatar Apr 06 '22 19:04 emmenlau

Just a kind reminder about this humble question. Let me know if I can do something to support this request?

emmenlau avatar Jul 31 '22 07:07 emmenlau

@emmenlau - thanks a lot for analysis and patches - be free to comment on - https://github.com/oneapi-src/oneDAL/pull/2230

napetrov avatar Jan 05 '23 21:01 napetrov