[Lldb-commits] [PATCH] D57282: [cmake] Fix get_llvm_lit_path() to respect LLVM_EXTERNAL_LIT always
mgorny created this revision.
mgorny added reviewers: EricWF, sgraenitz, zturner.
Herald added a subscriber: christof.
Refactor the get_llvm_lit_path() logic to respect LLVM_EXTERNAL_LIT,
and require the fallback to be defined explicitly
as LLVM_DEFAULT_EXTERNAL_LIT. This fixes building libcxx standalone
after r346888.
The old logic was using LLVM_EXTERNAL_LIT both as user-defined cache
variable and an optional pre-definition of default value from caller
(e.g. libcxx). It included a hack to make this work by assigning
the value back and forth but it was fragile and stopped working
in libcxx.
The new logic is simpler and more transparent. Default value is
provided in a separate variable, and used only when user-specified
variable is empty (i.e. not overriden).
https://reviews.llvm.org/D57282
Files:
libcxx/cmake/Modules/HandleOutOfTreeLLVM.cmake
libcxxabi/cmake/Modules/HandleOutOfTreeLLVM.cmake
lldb/cmake/modules/LLDBStandalone.cmake
llvm/cmake/modules/AddLLVM.cmake
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1280,7 +1280,6 @@
cmake_parse_arguments(ARG "ALLOW_EXTERNAL" "" "" ${ARGN})
if (ARG_ALLOW_EXTERNAL)
-set(LLVM_DEFAULT_EXTERNAL_LIT "${LLVM_EXTERNAL_LIT}")
set (LLVM_EXTERNAL_LIT "" CACHE STRING "Command used to spawn lit")
if ("${LLVM_EXTERNAL_LIT}" STREQUAL "")
set(LLVM_EXTERNAL_LIT "${LLVM_DEFAULT_EXTERNAL_LIT}")
Index: lldb/cmake/modules/LLDBStandalone.cmake
===
--- lldb/cmake/modules/LLDBStandalone.cmake
+++ lldb/cmake/modules/LLDBStandalone.cmake
@@ -58,7 +58,7 @@
set(LLVM_DIR ${LLVM_OBJ_ROOT}/cmake/modules/CMakeFiles CACHE PATH "Path to
LLVM build tree CMake files")
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")
- set(LLVM_EXTERNAL_LIT ${LLVM_TOOLS_BINARY_DIR}/llvm-lit CACHE PATH "Path to
llvm-lit")
+ set(LLVM_DEFAULT_EXTERNAL_LIT ${LLVM_TOOLS_BINARY_DIR}/llvm-lit CACHE PATH
"Path to llvm-lit")
find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
NO_DEFAULT_PATH)
Index: libcxxabi/cmake/Modules/HandleOutOfTreeLLVM.cmake
===
--- libcxxabi/cmake/Modules/HandleOutOfTreeLLVM.cmake
+++ libcxxabi/cmake/Modules/HandleOutOfTreeLLVM.cmake
@@ -117,7 +117,7 @@
# Required LIT Configuration
# Define the default arguments to use with 'lit', and an option for the
user
# to override.
-set(LLVM_EXTERNAL_LIT "${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py")
+set(LLVM_DEFAULT_EXTERNAL_LIT "${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py")
set(LIT_ARGS_DEFAULT "-sv --show-xfail --show-unsupported")
if (MSVC OR XCODE)
set(LIT_ARGS_DEFAULT "${LIT_ARGS_DEFAULT} --no-progress-bar")
Index: libcxx/cmake/Modules/HandleOutOfTreeLLVM.cmake
===
--- libcxx/cmake/Modules/HandleOutOfTreeLLVM.cmake
+++ libcxx/cmake/Modules/HandleOutOfTreeLLVM.cmake
@@ -116,7 +116,7 @@
# Required LIT Configuration
# Define the default arguments to use with 'lit', and an option for the
user
# to override.
-set(LLVM_EXTERNAL_LIT "${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py")
+set(LLVM_DEFAULT_EXTERNAL_LIT "${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py")
set(LIT_ARGS_DEFAULT "-sv --show-xfail --show-unsupported")
if (MSVC OR XCODE)
set(LIT_ARGS_DEFAULT "${LIT_ARGS_DEFAULT} --no-progress-bar")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1280,7 +1280,6 @@
cmake_parse_arguments(ARG "ALLOW_EXTERNAL" "" "" ${ARGN})
if (ARG_ALLOW_EXTERNAL)
-set(LLVM_DEFAULT_EXTERNAL_LIT "${LLVM_EXTERNAL_LIT}")
set (LLVM_EXTERNAL_LIT "" CACHE STRING "Command used to spawn lit")
if ("${LLVM_EXTERNAL_LIT}" STREQUAL "")
set(LLVM_EXTERNAL_LIT "${LLVM_DEFAULT_EXTERNAL_LIT}")
Index: lldb/cmake/modules/LLDBStandalone.cmake
===
--- lldb/cmake/modules/LLDBStandalone.cmake
+++ lldb/cmake/modules/LLDBStandalone.cmake
@@ -58,7 +58,7 @@
set(LLVM_DIR ${LLVM_OBJ_ROOT}/cmake/modules/CMakeFiles CACHE PATH "Path to LLVM build tree CMake files")
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")
- set(LLVM_EXTERNAL_LIT ${LLVM_TOOLS_BINARY_DIR}/llvm-lit CACHE PATH "Path to llvm-lit")
+ set(LLVM_DEFAULT_EXTERNAL_LIT ${LLVM_TOOLS_BINARY_DIR}/llvm-lit CACHE PATH "Path to llvm-lit")
find_
[Lldb-commits] [PATCH] D57275: [testsuite] Remove seven dependency
stella.stamenova added a comment. I don't have a strong preference between using a separate module for interopability between python versions and implementing the functions as part of dotest if there are few of them, but I have to agree with @zturner that if we have more than a couple of functions, it makes sense to add them to an explicit module. Incidentally, I think we do have more than a couple of functions but they are not in the module right now - most likely because people who added them (me included) didn't know any better. Comment at: packages/Python/lldbsuite/support/seven.py:18 -shell=True, -universal_newlines=True)) -except subprocess.CalledProcessError as e: I suspect that if you end up using your implementation, you'll have to add universal_newlines=True, so that it all works correctly across platform (and on Windows) Comment at: packages/Python/lldbsuite/test/dotest.py:51 +def get_command_output(cmd): + try: This version of get_command_output no longer uses six for python 2, does it still work correctly for both python 2 and python 3? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57275/new/ https://reviews.llvm.org/D57275 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D57272: Remove unimplemented function
davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57272/new/ https://reviews.llvm.org/D57272 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D57275: [testsuite] Remove seven dependency
On Fri, Jan 25, 2019 at 8:44 PM Zachary Turner via lldb-commits < lldb-commits@lists.llvm.org> wrote: > The idea behind seven is that it’s a place to put stuff that we need for > py2/py3 interoperability that doesn’t already exist in six. Yes, maybe > there’s only one thing there now, but there could be more over time. > > At least that was the thinking when I created it. When I was looking at this with Davide yesterday, we were under the impression that it was an external package (https://pypi.org/project/seven) or part thereof. Compatibility with Python 2.5 didn't sound particularly important which is why I wanted to remove it. Anyway, now that I know its purpose, I'm okay with fixing the newline issue there. > It seems like there’s two separate issues here: 1) you need to fix a bug, > and 2) you want to propose removing seven. Is it worth doing those two > things separately? > On Fri, Jan 25, 2019 at 6:00 PM Jonas Devlieghere via Phabricator < > revi...@reviews.llvm.org> wrote: > >> JDevlieghere created this revision. >> JDevlieghere added reviewers: davide, zturner, stella.stamenova, labath. >> JDevlieghere added a project: LLDB. >> Herald added a reviewer: serge-sans-paille. >> >> When running the test suite on macOS with Python 3 we noticed a >> difference in behavior between Python 2 and Python 3 for >> `seven.get_command_output`. The output contained a newline with Python 3, >> but not for Python 2. This resulted in an invalid SDK path passed to the >> compiler. >> >> There were only two actual usages left of this module so I propose to >> remove it and have a simple, local implementation for `get_command_output`. >> >> >> Repository: >> rLLDB LLDB >> >> https://reviews.llvm.org/D57275 >> >> Files: >> packages/Python/lldbsuite/support/seven.py >> packages/Python/lldbsuite/test/dosep.py >> packages/Python/lldbsuite/test/dotest.py >> packages/Python/lldbsuite/test/functionalities/exec/TestExec.py >> >> packages/Python/lldbsuite/test/functionalities/fat_archives/TestFatArchives.py >> packages/Python/lldbsuite/test/lang/objc/ivar-IMP/TestObjCiVarIMP.py >> >> ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D57275: [testsuite] Remove seven dependency
Huh, that’s a coincidence. I chose the name because it was “like six”, but I guess someone else chose it for the same reason On Sat, Jan 26, 2019 at 2:18 PM Jonas Devlieghere wrote: > > > On Fri, Jan 25, 2019 at 8:44 PM Zachary Turner via lldb-commits < > lldb-commits@lists.llvm.org> wrote: > >> The idea behind seven is that it’s a place to put stuff that we need for >> py2/py3 interoperability that doesn’t already exist in six. Yes, maybe >> there’s only one thing there now, but there could be more over time. >> >> At least that was the thinking when I created it. > > > When I was looking at this with Davide yesterday, we were under the > impression that it was an external package (https://pypi.org/project/seven) > or part thereof. Compatibility with Python 2.5 didn't sound particularly > important which is why I wanted to remove it. Anyway, now that I know its > purpose, I'm okay with fixing the newline issue there. > > >> It seems like there’s two separate issues here: 1) you need to fix a bug, >> and 2) you want to propose removing seven. Is it worth doing those two >> things separately? >> On Fri, Jan 25, 2019 at 6:00 PM Jonas Devlieghere via Phabricator < >> revi...@reviews.llvm.org> wrote: >> >>> JDevlieghere created this revision. >>> JDevlieghere added reviewers: davide, zturner, stella.stamenova, labath. >>> JDevlieghere added a project: LLDB. >>> Herald added a reviewer: serge-sans-paille. >>> >>> When running the test suite on macOS with Python 3 we noticed a >>> difference in behavior between Python 2 and Python 3 for >>> `seven.get_command_output`. The output contained a newline with Python 3, >>> but not for Python 2. This resulted in an invalid SDK path passed to the >>> compiler. >>> >>> There were only two actual usages left of this module so I propose to >>> remove it and have a simple, local implementation for `get_command_output`. >>> >>> >>> Repository: >>> rLLDB LLDB >>> >>> https://reviews.llvm.org/D57275 >>> >>> Files: >>> packages/Python/lldbsuite/support/seven.py >>> packages/Python/lldbsuite/test/dosep.py >>> packages/Python/lldbsuite/test/dotest.py >>> packages/Python/lldbsuite/test/functionalities/exec/TestExec.py >>> >>> packages/Python/lldbsuite/test/functionalities/fat_archives/TestFatArchives.py >>> packages/Python/lldbsuite/test/lang/objc/ivar-IMP/TestObjCiVarIMP.py >>> >>> ___ >> lldb-commits mailing list >> lldb-commits@lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits >> > ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
