[Openmp-commits] [PATCH] D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu

[George Rokos via Phabricator via Openmp-commits]

grokos accepted this revision.
grokos added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake:116
+if (CUDA_TOOLKIT_ROOT_DIR)
+  set(cuda_toolkit_root_dir_old TRUE)
+endif()
----------------
jdenny wrote:
> grokos wrote:
> > There's a convention that cmake variables are capitalized. Also, cmake variables local to libomptarget should start with the prefix `LIBOMPTARGET_`. So this variable's name should be changed to `LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET` ("preset" makes more sense than "old").
> Thanks for the review.  Sure, I'll make those changes.
> 
> However, I had thought that non-cached or local variables are usually lowercase.  Is there a distinction here I'm not seeing, or is that just a different style than you prefer here?
True, local variables are lowercase, but non-cached variables are still capitalized. For `LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET` and `LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR` I prefer the capitalized version because these variables may be used outside this file in the future (although right now they are only used locally).


================
Comment at: openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake:185
+      OUTPUT_STRIP_TRAILING_WHITESPACE)
+    set(candidate_dir /usr/lib/cuda)
+    if ((LSB_RELEASE_ID STREQUAL "Debian" OR LSB_RELEASE_ID STREQUAL "Ubuntu")
----------------
jdenny wrote:
> Should candidate_dir be LIBOMPTARGET_CANDIDATE_DIR?
It's OK, it's only local.


Repository:
  rOMP OpenMP

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55588/new/

https://reviews.llvm.org/D55588