[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice added a comment.

In D141907#4398196 , @protze.joachim 
wrote:

> The review should not be closed, since the patch was reverted and should be 
> recommitted (this time possibly with a reference to this review?)

This is recommitted as 0beffb854209a41f31beb18f9631258349a99299 
, but 
break some lldb builds, so I created https://reviews.llvm.org/D152225 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice added a comment.

In D141907#4361594 , @tstellar wrote:

> In D141907#4355229 , @paperchalice 
> wrote:
>
>> In D141907#4355228 , @tstellar 
>> wrote:
>>
>>> @paperchalice Do you need someone to commit this for you?
>>
>> Sure, I don't have the commit access.
>
> What name / email address do you want me to use for the commit author field?

Sorry for the late, the information in phabricator is OK, and 
`GetClangResourceDir.cmake` is not landed correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice added a comment.

In D141907#4355228 , @tstellar wrote:

> @paperchalice Do you need someone to commit this for you?

Sure, I don't have the commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice added inline comments.



Comment at: cmake/Modules/GetClangResourceDir.cmake:13
+  if(DEFINED CLANG_RESOURCE_DIR AND NOT CLANG_RESOURCE_DIR STREQUAL "")
+set(ret_dir bin/${CLANG_RESOURCE_DIR})
+  else()

tstellar wrote:
> tstellar wrote:
> > tstellar wrote:
> > > paperchalice wrote:
> > > > tstellar wrote:
> > > > > Why is the bin prefix here?
> > > > See 
> > > > https://clang.llvm.org/doxygen/classclang_1_1driver_1_1Driver.html#acda8dfdf4f80efa84df98157e1779152
> > > > `GetResourcesPath` will return `/lib/` when 
> > > > `CLANG_RESOURCE_DIR` is empty, `/bin/CLANG_RESOURCE_DIR` 
> > > > otherwise.
> > > > 
> > > `GetResourcesPath` calls `sys::path::parent_path` twice on the path to 
> > > the clang executable which is going to give you `/usr ` on most Linux 
> > > systems, so it's returning `/CLANG_RESOURCE_DIR` not 
> > > `/bin/CLANG_RESOURCE_DIR`.
> > Sorry, you are correct.  I was looking at the wrong branch.  It's really 
> > strange that it does that.
> @paperchalice Can you update the description of the CLANG_RESOURCE_DIR cache 
> variable in clang/CMakeLists.txt to mention that the path should be relative 
> to the directory with the clang executable.
Already in [[ 
https://github.com/llvm/llvm-project/blob/c5fefbc8dbc49d1c3133615b46fc9a3ca93dcdc2/clang/CMakeLists.txt#LL178C53-L178C53
 | codebase ]]:
```
set(CLANG_RESOURCE_DIR "" CACHE STRING
  "Relative directory from the Clang binary to its resource files.")
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice added inline comments.



Comment at: cmake/Modules/GetClangResourceDir.cmake:13
+  if(DEFINED CLANG_RESOURCE_DIR AND NOT CLANG_RESOURCE_DIR STREQUAL "")
+set(ret_dir bin/${CLANG_RESOURCE_DIR})
+  else()

tstellar wrote:
> Why is the bin prefix here?
See 
https://clang.llvm.org/doxygen/classclang_1_1driver_1_1Driver.html#acda8dfdf4f80efa84df98157e1779152
`GetResourcesPath` will return `/lib/` when 
`CLANG_RESOURCE_DIR` is empty, `/bin/CLANG_RESOURCE_DIR` otherwise.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice added a comment.

Ping.




Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp:56
+  const std::string clang_resource_path =
+  clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 

Should be "${CMAKE_INSTALL_BINDIR}/lldb" here, but need another variable in 
`config.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice updated this revision to Diff 494223.
paperchalice added a comment.

- Use `clang::driver::Driver::GetResourcesPath`
- Use `ARG` as prefix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

Files:
  clang/lib/Headers/CMakeLists.txt
  clang/lib/Tooling/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  cmake/Modules/GetClangResourceDir.cmake
  compiler-rt/cmake/base-config-ix.cmake
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/cmake/modules/LLVMExternalProjectUtils.cmake
  openmp/CMakeLists.txt

Index: openmp/CMakeLists.txt
===
--- openmp/CMakeLists.txt
+++ openmp/CMakeLists.txt
@@ -86,8 +86,8 @@
 if(${OPENMP_STANDALONE_BUILD})
   set(LIBOMP_HEADERS_INSTALL_PATH "${CMAKE_INSTALL_INCLUDEDIR}")
 else()
-  string(REGEX MATCH "[0-9]+" CLANG_VERSION ${PACKAGE_VERSION})
-  set(LIBOMP_HEADERS_INSTALL_PATH "${OPENMP_INSTALL_LIBDIR}/clang/${CLANG_VERSION}/include")
+  include(GetClangResourceDir)
+  get_clang_resource_dir(LIBOMP_HEADERS_INSTALL_PATH SUBDIR include)
 endif()
 
 # Build host runtime library, after LIBOMPTARGET variables are set since they are needed
Index: llvm/cmake/modules/LLVMExternalProjectUtils.cmake
===
--- llvm/cmake/modules/LLVMExternalProjectUtils.cmake
+++ llvm/cmake/modules/LLVMExternalProjectUtils.cmake
@@ -257,7 +257,11 @@
 if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
   string(REGEX MATCH "^[0-9]+" CLANG_VERSION_MAJOR
  ${PACKAGE_VERSION})
-  set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION_MAJOR}")
+  if(DEFINED CLANG_RESOURCE_DIR AND NOT CLANG_RESOURCE_DIR STREQUAL "")
+set(resource_dir ${LLVM_TOOLS_BINARY_DIR}/${CLANG_RESOURCE_DIR})
+  else()
+set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION_MAJOR}")
+  endif()
   set(flag_types ASM C CXX MODULE_LINKER SHARED_LINKER EXE_LINKER)
   foreach(type ${flag_types})
 set(${type}_flag -DCMAKE_${type}_FLAGS=-resource-dir=${resource_dir})
Index: lldb/unittests/Expression/ClangParserTest.cpp
===
--- lldb/unittests/Expression/ClangParserTest.cpp
+++ lldb/unittests/Expression/ClangParserTest.cpp
@@ -7,6 +7,8 @@
 //===--===//
 
 #include "clang/Basic/Version.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangHost.h"
 #include "TestingSupport/SubsystemRAII.h"
@@ -37,13 +39,11 @@
 TEST_F(ClangHostTest, ComputeClangResourceDirectory) {
 #if !defined(_WIN32)
   std::string path_to_liblldb = "/foo/bar/lib/";
-  std::string path_to_clang_dir =
-  "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING;
 #else
-  std::string path_to_liblldb = "C:\\foo\\bar\\lib";
-  std::string path_to_clang_dir =
-  "C:\\foo\\bar\\lib\\clang\\" CLANG_VERSION_MAJOR_STRING;
+  std::string path_to_liblldb = "C:\\foo\\bar\\lib\\";
 #endif
+  std::string path_to_clang_dir = clang::driver::Driver::GetResourcesPath(
+  path_to_liblldb + "liblldb", CLANG_RESOURCE_DIR);
   EXPECT_EQ(ComputeClangResourceDir(path_to_liblldb), path_to_clang_dir);
 
   // The path doesn't really exist, so setting verify to true should make
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -10,6 +10,7 @@
 
 #include "clang/Basic/Version.h"
 #include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
@@ -51,11 +52,14 @@
   Log *log = GetLog(LLDBLog::Host);
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
+  const std::string clang_resource_path =
+  clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 
   static const llvm::StringRef kResourceDirSuffixes[] = {
   // LLVM.org's build of LLDB uses the clang resource directory placed
-  // in $install_dir/lib{,64}/clang/$clang_version.
-  CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING,
+  // in $install_dir/lib{,64}/clang/$clang_version or
+  // $install_dir/bin/$CLANG_RESOURCE_DIR
+  clang_resource_path,
   // swift-lldb uses the clang resource directory copied from swift, which
   // by default is placed in $install_dir/lib{,64}/lldb/clang. LLDB places
   // it there, so we use LLDB_INSTALL_LIBDIR_BASENAME.
@@ -82,7 +86,8 @@
 }
 
 bool lldb_private::ComputeClangResourceDirectory(FileSpec &lldb_shlib_spec,
-  

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Junchang Liu via Phabricator via cfe-commits]

paperchalice added a comment.

In D141907#4094748 , @MaskRay wrote:

> The CMake part of this patch improves the matter. Manually constructed 
> resource dir (many duplicates) string is replace with a library function call.
>
> However, the introduced conditions in C++ code like
>
>   std::string path_to_clang_dir = std::string(CLANG_RESOURCE_DIR).empty()
>   ? "/foo/bar/" 
> LLDB_INSTALL_LIBDIR_BASENAME
> "/clang/" CLANG_VERSION_MAJOR_STRING
>   : "/foo/bar/bin/" CLANG_RESOURCE_DIR;
>
> makes me uncomfortable.
>
> I wish that we can just replace all manually constructed 
> `CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING` with a 
> simple `CLANG_RESOURCE_DIR` which is guaranteed to be non-empty.
>
> edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable 
> `CLANG_RESOURCE_DIR` but did not explain why. 
> In the long term, the CMake variable `CLANG_RESOURCE_DIR` probably should be 
> removed.

clang::driver::Driver::GetResourcesPath 

 would be a better choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits