[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

@nikic I have tried to address your concerns in https://reviews.llvm.org/D118792


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

In D116492#3290087 , @nikic wrote:

> Would it be possible to share a complete new-style cmake invocation that is 
> supposed to work?

Our standalone LLD package in Nixpkgs works, but I see now we have been passing 
`-DLLVM_MAIN_SRC_DIR` all along.

> If I only point LLVM_DIR to the cmake directory, then LLVM_MAIN_SRC_DIR ends 
> up being empty (it is set to MAIN_SRC_DIR, which, as far as I can tell, is 
> only initialized in the LLVM_CONFIG code path).

https://reviews.llvm.org/D51714 did just that, and it was never changed in the 
years since, so I figured I was missing something and it somehow worked.




Comment at: lld/CMakeLists.txt:14
+  set(LLVM_CONFIG_OUTPUT)
+  if(LLVM_CONFIG)
+set (LLVM_CONFIG_FOUND 1)

nikic wrote:
> Is it intentional that lld now requires you to set LLVM_CONFIG instead of 
> LLVM_CONFIG_PATH? Based on the deprecation, I would have assumed that the 
> previous name would be retained.
I was just following what Clang does, but I see now in 
https://reviews.llvm.org/D51714 that was preserving the existing name. I 
suppose this should be changed back, then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Would it be possible to share a complete new-style cmake invocation that is 
supposed to work? If I only point LLVM_DIR to the cmake directory, then 
LLVM_MAIN_SRC_DIR ends up being empty (it is set to MAIN_SRC_DIR, which, as far 
as I can tell, is only initialized in the LLVM_CONFIG code path).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: lld/CMakeLists.txt:14
+  set(LLVM_CONFIG_OUTPUT)
+  if(LLVM_CONFIG)
+set (LLVM_CONFIG_FOUND 1)

Is it intentional that lld now requires you to set LLVM_CONFIG instead of 
LLVM_CONFIG_PATH? Based on the deprecation, I would have assumed that the 
previous name would be retained.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa1da5f3c2d65: [lld] Deprecate using llvm-config to detect 
llvm installation (authored by Ericson2314).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

Files:
  lld/CMakeLists.txt

Index: lld/CMakeLists.txt
===
--- lld/CMakeLists.txt
+++ lld/CMakeLists.txt
@@ -1,58 +1,77 @@
+cmake_minimum_required(VERSION 3.13.4)
+
 include(GNUInstallDirs)
 
-# Check if lld is built as a standalone project.
+# If we are not building as a part of LLVM, build LLD as an
+# standalone project, using LLVM as an external library:
 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   project(lld)
-  cmake_minimum_required(VERSION 3.13.4)
 
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  set(LLD_BUILT_STANDALONE TRUE)
 
-  find_program(LLVM_CONFIG_PATH "llvm-config" DOC "Path to llvm-config binary")
-  if(NOT LLVM_CONFIG_PATH)
-message(FATAL_ERROR "llvm-config not found: specify LLVM_CONFIG_PATH")
-  endif()
+  # Rely on llvm-config.
+  set(LLVM_CONFIG_OUTPUT)
+  if(LLVM_CONFIG)
+set (LLVM_CONFIG_FOUND 1)
+message(STATUS "Found LLVM_CONFIG as ${LLVM_CONFIG}")
+message(DEPRECATION "Using llvm-config to detect the LLVM installation is \
+  deprecated.  The installed cmake files should be used \
+  instead.  CMake should be able to detect your LLVM install \
+  automatically, but you can also use LLVM_DIR to specify \
+  the path containing LLVMConfig.cmake.")
+set(CONFIG_COMMAND ${LLVM_CONFIG}
+  "--includedir"
+  "--prefix"
+  "--src-root"
+  "--cmakedir"
+  )
+execute_process(
+  COMMAND ${CONFIG_COMMAND}
+  RESULT_VARIABLE HAD_ERROR
+  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
+)
+if(NOT HAD_ERROR)
+  string(REGEX REPLACE
+"[ \t]*[\r\n]+[ \t]*" ";"
+LLVM_CONFIG_OUTPUT ${LLVM_CONFIG_OUTPUT})
+else()
+  string(REPLACE ";" " " CONFIG_COMMAND_STR "${CONFIG_COMMAND}")
+  message(STATUS "${CONFIG_COMMAND_STR}")
+  message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
+endif()
+
+list(GET LLVM_CONFIG_OUTPUT 0 MAIN_INCLUDE_DIR)
+list(GET LLVM_CONFIG_OUTPUT 1 LLVM_OBJ_ROOT)
+list(GET LLVM_CONFIG_OUTPUT 2 MAIN_SRC_DIR)
+list(GET LLVM_CONFIG_OUTPUT 3 LLVM_CONFIG_CMAKE_DIR)
 
-  execute_process(COMMAND "${LLVM_CONFIG_PATH}"
-  "--obj-root"
-  "--includedir"
-  "--cmakedir"
-  "--src-root"
-  RESULT_VARIABLE HAD_ERROR
-  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
-  OUTPUT_STRIP_TRAILING_WHITESPACE)
-  if(HAD_ERROR)
-message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
+# Normalize LLVM_CMAKE_DIR. --cmakedir might contain backslashes.
+# CMake assumes slashes as PATH.
+file(TO_CMAKE_PATH ${LLVM_CONFIG_CMAKE_DIR} LLVM_CMAKE_DIR)
   endif()
 
-  string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" LLVM_CONFIG_OUTPUT "${LLVM_CONFIG_OUTPUT}")
+  find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_DIR}")
+  list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})
 
-  list(GET LLVM_CONFIG_OUTPUT 0 OBJ_ROOT)
-  list(GET LLVM_CONFIG_OUTPUT 1 MAIN_INCLUDE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 2 LLVM_CMAKE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 3 MAIN_SRC_DIR)
+  # We can't check LLVM_CONFIG here, because find_package(LLVM ...) also sets
+  # LLVM_CONFIG.
+  if (NOT LLVM_CONFIG_FOUND)
+# Pull values from LLVMConfig.cmake.  We can drop this once the llvm-config
+# path is removed.
+set(MAIN_INCLUDE_DIR ${LLVM_INCLUDE_DIR})
+set(LLVM_OBJ_DIR ${LLVM_BINARY_DIR})
+  endif()
 
-  set(LLVM_OBJ_ROOT ${OBJ_ROOT} CACHE PATH "path to LLVM build tree")
-  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "path to llvm/include")
+  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")
+  set(LLVM_BINARY_DIR ${LLVM_OBJ_ROOT} CACHE PATH "Path to LLVM build tree")
   set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
 
-  file(TO_CMAKE_PATH "${LLVM_OBJ_ROOT}" LLVM_BINARY_DIR)
-  file(TO_CMAKE_PATH "${LLVM_CMAKE_DIR}" LLVM_CMAKE_DIR)
-
-  if(NOT EXISTS "${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
-message(FATAL_ERROR "LLVMConfig.cmake not found")
-  endif()
-  include("${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
-
-  list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_DIR}")
+  find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
+NO_DEFAULT_PATH)
 
-  set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
-  include_directories("${LLVM_BINARY_DIR}/include" ${LLVM_INCLUDE_DIRS})
-  link_directories(${LLVM_LIBRARY_DIRS})
-
-  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  # They are used as destination of target 

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[Chris Bieneman via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added inline comments.



Comment at: lld/CMakeLists.txt:17
-  execute_process(COMMAND "${LLVM_CONFIG_PATH}"
-  "--obj-root"
-  "--includedir"

`--obj-root` is the same thing as `--prefix`, so this is in fact a mere 
reordering in behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 marked an inline comment as done.
Ericson2314 added inline comments.



Comment at: clang/CMakeLists.txt:26
   "--src-root"
-  "--cmakedir")
+  "--cmakedir"
+  "--bindir"

Ericson2314 wrote:
> beanz wrote:
> > Ericson2314 wrote:
> > > beanz wrote:
> > > > I assume these are re-arranged to match what you're doing in lld. Is 
> > > > that correct?
> > > > 
> > > > Generally it is preferred to do this kind of non-functional 
> > > > restructuring in a separate commit from the commit with a functional 
> > > > change to make it easier to review the functional changes.
> > > > 
> > > > This patch as-is seems to have cleanup to clang, and a functional 
> > > > change for lld. Those should likely be two separate commits.
> > > Happy to split up.
> > > 
> > > The clang changes I wouldn't even say make it "better" per say, I am just 
> > > reording things to put the stuff LLD also goes first for sake diffing the 
> > > two. I thought that might look lke a noisy and pointless patch on its 
> > > own, but I am happy to split it up if you still think it's a good idea.
> > More smaller patches is generally the LLVM philosophy, so I do think it 
> > makes sense to split it up into an NFC patch for clang, and a separate 
> > change for lld.
> Sure. Done in D116492.
Oops, that's this one! I meant D116548.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 marked 2 inline comments as done.
Ericson2314 added inline comments.



Comment at: clang/CMakeLists.txt:26
   "--src-root"
-  "--cmakedir")
+  "--cmakedir"
+  "--bindir"

beanz wrote:
> Ericson2314 wrote:
> > beanz wrote:
> > > I assume these are re-arranged to match what you're doing in lld. Is that 
> > > correct?
> > > 
> > > Generally it is preferred to do this kind of non-functional restructuring 
> > > in a separate commit from the commit with a functional change to make it 
> > > easier to review the functional changes.
> > > 
> > > This patch as-is seems to have cleanup to clang, and a functional change 
> > > for lld. Those should likely be two separate commits.
> > Happy to split up.
> > 
> > The clang changes I wouldn't even say make it "better" per say, I am just 
> > reording things to put the stuff LLD also goes first for sake diffing the 
> > two. I thought that might look lke a noisy and pointless patch on its own, 
> > but I am happy to split it up if you still think it's a good idea.
> More smaller patches is generally the LLVM philosophy, so I do think it makes 
> sense to split it up into an NFC patch for clang, and a separate change for 
> lld.
Sure. Done in D116492.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 updated this revision to Diff 397100.
Ericson2314 added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Split out Clang changes into separate commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

Files:
  lld/CMakeLists.txt

Index: lld/CMakeLists.txt
===
--- lld/CMakeLists.txt
+++ lld/CMakeLists.txt
@@ -1,58 +1,77 @@
+cmake_minimum_required(VERSION 3.13.4)
+
 include(GNUInstallDirs)
 
-# Check if lld is built as a standalone project.
+# If we are not building as a part of LLVM, build LLD as an
+# standalone project, using LLVM as an external library:
 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   project(lld)
-  cmake_minimum_required(VERSION 3.13.4)
 
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  set(LLD_BUILT_STANDALONE TRUE)
 
-  find_program(LLVM_CONFIG_PATH "llvm-config" DOC "Path to llvm-config binary")
-  if(NOT LLVM_CONFIG_PATH)
-message(FATAL_ERROR "llvm-config not found: specify LLVM_CONFIG_PATH")
-  endif()
+  # Rely on llvm-config.
+  set(LLVM_CONFIG_OUTPUT)
+  if(LLVM_CONFIG)
+set (LLVM_CONFIG_FOUND 1)
+message(STATUS "Found LLVM_CONFIG as ${LLVM_CONFIG}")
+message(DEPRECATION "Using llvm-config to detect the LLVM installation is \
+  deprecated.  The installed cmake files should be used \
+  instead.  CMake should be able to detect your LLVM install \
+  automatically, but you can also use LLVM_DIR to specify \
+  the path containing LLVMConfig.cmake.")
+set(CONFIG_COMMAND ${LLVM_CONFIG}
+  "--includedir"
+  "--prefix"
+  "--src-root"
+  "--cmakedir"
+  )
+execute_process(
+  COMMAND ${CONFIG_COMMAND}
+  RESULT_VARIABLE HAD_ERROR
+  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
+)
+if(NOT HAD_ERROR)
+  string(REGEX REPLACE
+"[ \t]*[\r\n]+[ \t]*" ";"
+LLVM_CONFIG_OUTPUT ${LLVM_CONFIG_OUTPUT})
+else()
+  string(REPLACE ";" " " CONFIG_COMMAND_STR "${CONFIG_COMMAND}")
+  message(STATUS "${CONFIG_COMMAND_STR}")
+  message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
+endif()
+
+list(GET LLVM_CONFIG_OUTPUT 0 MAIN_INCLUDE_DIR)
+list(GET LLVM_CONFIG_OUTPUT 1 LLVM_OBJ_ROOT)
+list(GET LLVM_CONFIG_OUTPUT 2 MAIN_SRC_DIR)
+list(GET LLVM_CONFIG_OUTPUT 3 LLVM_CONFIG_CMAKE_DIR)
 
-  execute_process(COMMAND "${LLVM_CONFIG_PATH}"
-  "--obj-root"
-  "--includedir"
-  "--cmakedir"
-  "--src-root"
-  RESULT_VARIABLE HAD_ERROR
-  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
-  OUTPUT_STRIP_TRAILING_WHITESPACE)
-  if(HAD_ERROR)
-message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
+# Normalize LLVM_CMAKE_DIR. --cmakedir might contain backslashes.
+# CMake assumes slashes as PATH.
+file(TO_CMAKE_PATH ${LLVM_CONFIG_CMAKE_DIR} LLVM_CMAKE_DIR)
   endif()
 
-  string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" LLVM_CONFIG_OUTPUT "${LLVM_CONFIG_OUTPUT}")
+  find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_DIR}")
+  list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})
 
-  list(GET LLVM_CONFIG_OUTPUT 0 OBJ_ROOT)
-  list(GET LLVM_CONFIG_OUTPUT 1 MAIN_INCLUDE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 2 LLVM_CMAKE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 3 MAIN_SRC_DIR)
+  # We can't check LLVM_CONFIG here, because find_package(LLVM ...) also sets
+  # LLVM_CONFIG.
+  if (NOT LLVM_CONFIG_FOUND)
+# Pull values from LLVMConfig.cmake.  We can drop this once the llvm-config
+# path is removed.
+set(MAIN_INCLUDE_DIR ${LLVM_INCLUDE_DIR})
+set(LLVM_OBJ_DIR ${LLVM_BINARY_DIR})
+  endif()
 
-  set(LLVM_OBJ_ROOT ${OBJ_ROOT} CACHE PATH "path to LLVM build tree")
-  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "path to llvm/include")
+  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")
+  set(LLVM_BINARY_DIR ${LLVM_OBJ_ROOT} CACHE PATH "Path to LLVM build tree")
   set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
 
-  file(TO_CMAKE_PATH "${LLVM_OBJ_ROOT}" LLVM_BINARY_DIR)
-  file(TO_CMAKE_PATH "${LLVM_CMAKE_DIR}" LLVM_CMAKE_DIR)
-
-  if(NOT EXISTS "${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
-message(FATAL_ERROR "LLVMConfig.cmake not found")
-  endif()
-  include("${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
-
-  list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_DIR}")
+  find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
+NO_DEFAULT_PATH)
 
-  set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
-  include_directories("${LLVM_BINARY_DIR}/include" ${LLVM_INCLUDE_DIRS})
-  link_directories(${LLVM_LIBRARY_DIRS})
-
-  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  # They are used as destination of target 

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/CMakeLists.txt:26
   "--src-root"
-  "--cmakedir")
+  "--cmakedir"
+  "--bindir"

Ericson2314 wrote:
> beanz wrote:
> > I assume these are re-arranged to match what you're doing in lld. Is that 
> > correct?
> > 
> > Generally it is preferred to do this kind of non-functional restructuring 
> > in a separate commit from the commit with a functional change to make it 
> > easier to review the functional changes.
> > 
> > This patch as-is seems to have cleanup to clang, and a functional change 
> > for lld. Those should likely be two separate commits.
> Happy to split up.
> 
> The clang changes I wouldn't even say make it "better" per say, I am just 
> reording things to put the stuff LLD also goes first for sake diffing the 
> two. I thought that might look lke a noisy and pointless patch on its own, 
> but I am happy to split it up if you still think it's a good idea.
More smaller patches is generally the LLVM philosophy, so I do think it makes 
sense to split it up into an NFC patch for clang, and a separate change for lld.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added inline comments.



Comment at: clang/CMakeLists.txt:26
   "--src-root"
-  "--cmakedir")
+  "--cmakedir"
+  "--bindir"

beanz wrote:
> I assume these are re-arranged to match what you're doing in lld. Is that 
> correct?
> 
> Generally it is preferred to do this kind of non-functional restructuring in 
> a separate commit from the commit with a functional change to make it easier 
> to review the functional changes.
> 
> This patch as-is seems to have cleanup to clang, and a functional change for 
> lld. Those should likely be two separate commits.
Happy to split up.

The clang changes I wouldn't even say make it "better" per say, I am just 
reording things to put the stuff LLD also goes first for sake diffing the two. 
I thought that might look lke a noisy and pointless patch on its own, but I am 
happy to split it up if you still think it's a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/CMakeLists.txt:26
   "--src-root"
-  "--cmakedir")
+  "--cmakedir"
+  "--bindir"

I assume these are re-arranged to match what you're doing in lld. Is that 
correct?

Generally it is preferred to do this kind of non-functional restructuring in a 
separate commit from the commit with a functional change to make it easier to 
review the functional changes.

This patch as-is seems to have cleanup to clang, and a functional change for 
lld. Those should likely be two separate commits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

Note this is a first draft, I need to go test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116492

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

[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

[John Ericson via Phabricator via cfe-commits]

Ericson2314 created this revision.
Ericson2314 added a reviewer: tstellar.
Herald added a subscriber: mgorny.
Ericson2314 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is continuing in the path of D51714 , 
which did this for Clang.

I have rearranged the source code Clang so one can diff the top-level
CMakeLists.txt of Clang and LLD, ensuring we use the same strategy for
both.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116492

Files:
  clang/CMakeLists.txt
  lld/CMakeLists.txt

Index: lld/CMakeLists.txt
===
--- lld/CMakeLists.txt
+++ lld/CMakeLists.txt
@@ -1,58 +1,77 @@
+cmake_minimum_required(VERSION 3.13.4)
+
 include(GNUInstallDirs)
 
-# Check if lld is built as a standalone project.
+# If we are not building as a part of LLVM, build LLD as an
+# standalone project, using LLVM as an external library:
 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   project(lld)
-  cmake_minimum_required(VERSION 3.13.4)
 
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  set(LLD_BUILT_STANDALONE TRUE)
 
-  find_program(LLVM_CONFIG_PATH "llvm-config" DOC "Path to llvm-config binary")
-  if(NOT LLVM_CONFIG_PATH)
-message(FATAL_ERROR "llvm-config not found: specify LLVM_CONFIG_PATH")
-  endif()
+  # Rely on llvm-config.
+  set(LLVM_CONFIG_OUTPUT)
+  if(LLVM_CONFIG)
+set (LLVM_CONFIG_FOUND 1)
+message(STATUS "Found LLVM_CONFIG as ${LLVM_CONFIG}")
+message(DEPRECATION "Using llvm-config to detect the LLVM installation is \
+  deprecated.  The installed cmake files should be used \
+  instead.  CMake should be able to detect your LLVM install \
+  automatically, but you can also use LLVM_DIR to specify \
+  the path containing LLVMConfig.cmake.")
+set(CONFIG_COMMAND ${LLVM_CONFIG}
+  "--includedir"
+  "--prefix"
+  "--src-root"
+  "--cmakedir"
+  )
+execute_process(
+  COMMAND ${CONFIG_COMMAND}
+  RESULT_VARIABLE HAD_ERROR
+  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
+)
+if(NOT HAD_ERROR)
+  string(REGEX REPLACE
+"[ \t]*[\r\n]+[ \t]*" ";"
+LLVM_CONFIG_OUTPUT ${LLVM_CONFIG_OUTPUT})
+else()
+  string(REPLACE ";" " " CONFIG_COMMAND_STR "${CONFIG_COMMAND}")
+  message(STATUS "${CONFIG_COMMAND_STR}")
+  message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
+endif()
+
+list(GET LLVM_CONFIG_OUTPUT 0 MAIN_INCLUDE_DIR)
+list(GET LLVM_CONFIG_OUTPUT 1 LLVM_OBJ_ROOT)
+list(GET LLVM_CONFIG_OUTPUT 2 MAIN_SRC_DIR)
+list(GET LLVM_CONFIG_OUTPUT 3 LLVM_CONFIG_CMAKE_DIR)
 
-  execute_process(COMMAND "${LLVM_CONFIG_PATH}"
-  "--obj-root"
-  "--includedir"
-  "--cmakedir"
-  "--src-root"
-  RESULT_VARIABLE HAD_ERROR
-  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
-  OUTPUT_STRIP_TRAILING_WHITESPACE)
-  if(HAD_ERROR)
-message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
+# Normalize LLVM_CMAKE_DIR. --cmakedir might contain backslashes.
+# CMake assumes slashes as PATH.
+file(TO_CMAKE_PATH ${LLVM_CONFIG_CMAKE_DIR} LLVM_CMAKE_DIR)
   endif()
 
-  string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" LLVM_CONFIG_OUTPUT "${LLVM_CONFIG_OUTPUT}")
+  find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_DIR}")
+  list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})
 
-  list(GET LLVM_CONFIG_OUTPUT 0 OBJ_ROOT)
-  list(GET LLVM_CONFIG_OUTPUT 1 MAIN_INCLUDE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 2 LLVM_CMAKE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 3 MAIN_SRC_DIR)
+  # We can't check LLVM_CONFIG here, because find_package(LLVM ...) also sets
+  # LLVM_CONFIG.
+  if (NOT LLVM_CONFIG_FOUND)
+# Pull values from LLVMConfig.cmake.  We can drop this once the llvm-config
+# path is removed.
+set(MAIN_INCLUDE_DIR ${LLVM_INCLUDE_DIR})
+set(LLVM_OBJ_DIR ${LLVM_BINARY_DIR})
+  endif()
 
-  set(LLVM_OBJ_ROOT ${OBJ_ROOT} CACHE PATH "path to LLVM build tree")
-  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "path to llvm/include")
+  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")
+  set(LLVM_BINARY_DIR ${LLVM_OBJ_ROOT} CACHE PATH "Path to LLVM build tree")
   set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
 
-  file(TO_CMAKE_PATH "${LLVM_OBJ_ROOT}" LLVM_BINARY_DIR)
-  file(TO_CMAKE_PATH "${LLVM_CMAKE_DIR}" LLVM_CMAKE_DIR)
-
-  if(NOT EXISTS "${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
-message(FATAL_ERROR "LLVMConfig.cmake not found")
-  endif()
-  include("${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
-
-  list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_DIR}")
+  find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
+NO_DEFAULT_PATH)
 
-  set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
-