subject:"\[Lldb\-commits\] \[PATCH\] D68442\: \[lldb\] Unifying lldb python path"

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Haibo Huang via Phabricator via lldb-commits

hhb closed this revision.
hhb added a comment.

This is merged as 61f471a 
 and 
0016b45 . 
Closing...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Haibo Huang via Phabricator via lldb-commits

hhb updated this revision to Diff 223701.
hhb added a comment.

Reverts the change related to python dir for windows.

FileSpec should always contain normalized path. I.e. using '/' even in windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442

Files:
  lldb/CMakeLists.txt
  lldb/scripts/CMakeLists.txt
  lldb/scripts/Python/finishSwigPythonLLDB.py
  lldb/scripts/finishSwigWrapperClasses.py
  lldb/scripts/get_relative_lib_dir.py
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -48,8 +48,7 @@
 
 protected:
   static void ComputePythonDirForApple(llvm::SmallVectorImpl );
-  static void ComputePythonDirForPosix(llvm::SmallVectorImpl );
-  static void ComputePythonDirForWindows(llvm::SmallVectorImpl );
+  static void ComputePythonDir(llvm::SmallVectorImpl );
 };
 } // namespace lldb_private
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -305,39 +305,26 @@
   auto rend = llvm::sys::path::rend(path_ref);
   auto framework = std::find(rbegin, rend, "LLDB.framework");
   if (framework == rend) {
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 return;
   }
   path.resize(framework - rend);
   llvm::sys::path::append(path, style, "LLDB.framework", "Resources", "Python");
 }
 
-void ScriptInterpreterPython::ComputePythonDirForPosix(
+void ScriptInterpreterPython::ComputePythonDir(
 llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::posix;
-#if defined(LLDB_PYTHON_RELATIVE_LIBDIR)
   // Build the path by backing out of the lib dir, then building with whatever
   // the real python interpreter uses.  (e.g. lib for most, lib64 on RHEL
-  // x86_64).
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, LLDB_PYTHON_RELATIVE_LIBDIR);
-#else
-  llvm::sys::path::append(path, style,
-  "python" + llvm::Twine(PY_MAJOR_VERSION) + "." +
-  llvm::Twine(PY_MINOR_VERSION),
-  "site-packages");
-#endif
-}
-
-void ScriptInterpreterPython::ComputePythonDirForWindows(
-llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::windows;
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, "lib", "site-packages");
+  // x86_64, or bin on Windows).
+  llvm::sys::path::remove_filename(path);
+  llvm::sys::path::append(path, LLDB_PYTHON_RELATIVE_LIBDIR);
 
+#if defined(_WIN32)
   // This will be injected directly through FileSpec.GetDirectory().SetString(),
   // so we need to normalize manually.
   std::replace(path.begin(), path.end(), '\\', '/');
+#endif
 }
 
 FileSpec ScriptInterpreterPython::GetPythonDir() {
@@ -350,10 +337,8 @@
 
 #if defined(__APPLE__)
 ComputePythonDirForApple(path);
-#elif defined(_WIN32)
-ComputePythonDirForWindows(path);
 #else
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 #endif
 spec.GetDirectory().SetString(path);
 return spec;
Index: lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
@@ -1,15 +1,7 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
-  # Call a python script to gather the arch-specific libdir for
-  # modules like the lldb module.
-  execute_process(
-COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../../../../scripts/get_relative_lib_dir.py
-RESULT_VARIABLE get_libdir_status
-OUTPUT_VARIABLE relative_libdir
-)
-  if (get_libdir_status EQUAL 0)
-add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${relative_libdir}")
-  endif()
+if(NOT LLDB_PYTHON_RELATIVE_PATH)
+  message(FATAL_ERROR "LLDB_PYTHON_RELATIVE_PATH is not set.")
 endif()
+add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${LLDB_PYTHON_RELATIVE_PATH}")
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
Index: lldb/scripts/get_relative_lib_dir.py
===
--- lldb/scripts/get_relative_lib_dir.py
+++ /dev/null
@@ -1,44 +0,0 @@
-import distutils.sysconfig
-import os
-import platform

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Haibo Huang via Phabricator via lldb-commits

hhb updated this revision to Diff 223689.
hhb added a comment.

Converts python output path to cmake format.

This looks like a bug on windows exists before this change...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442

Files:
  lldb/CMakeLists.txt
  lldb/scripts/CMakeLists.txt
  lldb/scripts/Python/finishSwigPythonLLDB.py
  lldb/scripts/finishSwigWrapperClasses.py
  lldb/scripts/get_relative_lib_dir.py
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -48,8 +48,7 @@
 
 protected:
   static void ComputePythonDirForApple(llvm::SmallVectorImpl );
-  static void ComputePythonDirForPosix(llvm::SmallVectorImpl );
-  static void ComputePythonDirForWindows(llvm::SmallVectorImpl );
+  static void ComputePythonDir(llvm::SmallVectorImpl );
 };
 } // namespace lldb_private
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -305,39 +305,20 @@
   auto rend = llvm::sys::path::rend(path_ref);
   auto framework = std::find(rbegin, rend, "LLDB.framework");
   if (framework == rend) {
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 return;
   }
   path.resize(framework - rend);
   llvm::sys::path::append(path, style, "LLDB.framework", "Resources", "Python");
 }
 
-void ScriptInterpreterPython::ComputePythonDirForPosix(
+void ScriptInterpreterPython::ComputePythonDir(
 llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::posix;
-#if defined(LLDB_PYTHON_RELATIVE_LIBDIR)
   // Build the path by backing out of the lib dir, then building with whatever
   // the real python interpreter uses.  (e.g. lib for most, lib64 on RHEL
-  // x86_64).
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, LLDB_PYTHON_RELATIVE_LIBDIR);
-#else
-  llvm::sys::path::append(path, style,
-  "python" + llvm::Twine(PY_MAJOR_VERSION) + "." +
-  llvm::Twine(PY_MINOR_VERSION),
-  "site-packages");
-#endif
-}
-
-void ScriptInterpreterPython::ComputePythonDirForWindows(
-llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::windows;
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, "lib", "site-packages");
-
-  // This will be injected directly through FileSpec.GetDirectory().SetString(),
-  // so we need to normalize manually.
-  std::replace(path.begin(), path.end(), '\\', '/');
+  // x86_64, or bin on Windows).
+  llvm::sys::path::remove_filename(path);
+  llvm::sys::path::append(path, LLDB_PYTHON_RELATIVE_LIBDIR);
 }
 
 FileSpec ScriptInterpreterPython::GetPythonDir() {
@@ -350,11 +331,10 @@
 
 #if defined(__APPLE__)
 ComputePythonDirForApple(path);
-#elif defined(_WIN32)
-ComputePythonDirForWindows(path);
 #else
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 #endif
+llvm::sys::path::native(path);
 spec.GetDirectory().SetString(path);
 return spec;
   }();
Index: lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
@@ -1,15 +1,7 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
-  # Call a python script to gather the arch-specific libdir for
-  # modules like the lldb module.
-  execute_process(
-COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../../../../scripts/get_relative_lib_dir.py
-RESULT_VARIABLE get_libdir_status
-OUTPUT_VARIABLE relative_libdir
-)
-  if (get_libdir_status EQUAL 0)
-add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${relative_libdir}")
-  endif()
+if(NOT LLDB_PYTHON_RELATIVE_PATH)
+  message(FATAL_ERROR "LLDB_PYTHON_RELATIVE_PATH is not set.")
 endif()
+add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${LLDB_PYTHON_RELATIVE_PATH}")
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
Index: lldb/scripts/get_relative_lib_dir.py
===
--- lldb/scripts/get_relative_lib_dir.py
+++ /dev/null
@@ -1,44 +0,0 @@
-import distutils.sysconfig
-import os
-import platform
-import re

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Haibo Huang via Phabricator via lldb-commits

hhb added a comment.

In D68442#1698053 , @mgorny wrote:

> Cool work. I presume you've tested it. I can test it tomorrow if you need me 
> to. However, I can do that after the commit.


I'm doing a final round of testing on linux/darwin/windows(mingw). I'll commit 
after that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Michał Górny via Phabricator via lldb-commits

mgorny accepted this revision.
mgorny added a comment.

Cool work. I presume you've tested it. I can test it tomorrow if you need me 
to. However, I can do that after the commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Haibo Huang via Phabricator via lldb-commits

hhb updated this revision to Diff 223638.
hhb marked an inline comment as done.
hhb added a comment.

Fix comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442

Files:
  lldb/CMakeLists.txt
  lldb/scripts/CMakeLists.txt
  lldb/scripts/Python/finishSwigPythonLLDB.py
  lldb/scripts/finishSwigWrapperClasses.py
  lldb/scripts/get_relative_lib_dir.py
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -48,8 +48,7 @@
 
 protected:
   static void ComputePythonDirForApple(llvm::SmallVectorImpl );
-  static void ComputePythonDirForPosix(llvm::SmallVectorImpl );
-  static void ComputePythonDirForWindows(llvm::SmallVectorImpl );
+  static void ComputePythonDir(llvm::SmallVectorImpl );
 };
 } // namespace lldb_private
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -305,39 +305,20 @@
   auto rend = llvm::sys::path::rend(path_ref);
   auto framework = std::find(rbegin, rend, "LLDB.framework");
   if (framework == rend) {
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 return;
   }
   path.resize(framework - rend);
   llvm::sys::path::append(path, style, "LLDB.framework", "Resources", "Python");
 }
 
-void ScriptInterpreterPython::ComputePythonDirForPosix(
+void ScriptInterpreterPython::ComputePythonDir(
 llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::posix;
-#if defined(LLDB_PYTHON_RELATIVE_LIBDIR)
   // Build the path by backing out of the lib dir, then building with whatever
   // the real python interpreter uses.  (e.g. lib for most, lib64 on RHEL
-  // x86_64).
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, LLDB_PYTHON_RELATIVE_LIBDIR);
-#else
-  llvm::sys::path::append(path, style,
-  "python" + llvm::Twine(PY_MAJOR_VERSION) + "." +
-  llvm::Twine(PY_MINOR_VERSION),
-  "site-packages");
-#endif
-}
-
-void ScriptInterpreterPython::ComputePythonDirForWindows(
-llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::windows;
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, "lib", "site-packages");
-
-  // This will be injected directly through FileSpec.GetDirectory().SetString(),
-  // so we need to normalize manually.
-  std::replace(path.begin(), path.end(), '\\', '/');
+  // x86_64, or bin on Windows).
+  llvm::sys::path::remove_filename(path);
+  llvm::sys::path::append(path, LLDB_PYTHON_RELATIVE_LIBDIR);
 }
 
 FileSpec ScriptInterpreterPython::GetPythonDir() {
@@ -350,11 +331,10 @@
 
 #if defined(__APPLE__)
 ComputePythonDirForApple(path);
-#elif defined(_WIN32)
-ComputePythonDirForWindows(path);
 #else
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 #endif
+llvm::sys::path::native(path);
 spec.GetDirectory().SetString(path);
 return spec;
   }();
Index: lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
@@ -1,15 +1,7 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
-  # Call a python script to gather the arch-specific libdir for
-  # modules like the lldb module.
-  execute_process(
-COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../../../../scripts/get_relative_lib_dir.py
-RESULT_VARIABLE get_libdir_status
-OUTPUT_VARIABLE relative_libdir
-)
-  if (get_libdir_status EQUAL 0)
-add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${relative_libdir}")
-  endif()
+if(NOT LLDB_PYTHON_RELATIVE_PATH)
+  message(FATAL_ERROR "LLDB_PYTHON_RELATIVE_PATH is not set.")
 endif()
+add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${LLDB_PYTHON_RELATIVE_PATH}")
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
Index: lldb/scripts/get_relative_lib_dir.py
===
--- lldb/scripts/get_relative_lib_dir.py
+++ /dev/null
@@ -1,44 +0,0 @@
-import distutils.sysconfig
-import os
-import platform
-import re
-import sys
-
-
-def get_python_relative_libdir():
-

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Haibo Huang via Phabricator via lldb-commits

hhb marked 3 inline comments as done and an inline comment as not done.
hhb added inline comments.



Comment at: lldb/CMakeLists.txt:42
+COMMAND ${PYTHON_EXECUTABLE}
+-c "import distutils.sysconfig, sys; 
print(distutils.sysconfig.get_python_lib(True, False, sys.argv[1]))"
+${CMAKE_INSTALL_PREFIX}

labath wrote:
> mgorny wrote:
> > hhb wrote:
> > > hhb wrote:
> > > > mgorny wrote:
> > > > > I still like my `(False, False, '')` version better than having to 
> > > > > recalculate path afterwards.
> > > > I don't have a strong opinion here. Let's see what labath@ think.
> > > > 
> > > > That been said, I did this because some distribution modified 
> > > > get_python_lib() to return differently based on prefix. One example is 
> > > > Debian/Ubuntu, where 'dist-packages' will be used if prefix is '', 
> > > > '/usr' or '/usr/local'.
> > > > 
> > > > In reality, that only makes difference when CMAKE_INSTALL_PREFIX is 
> > > > set. But I guess it doesn't really matter whether we use 
> > > > 'dist-packages' or 'site-packages' that time, as long as it is 
> > > > consistent everywhere.
> > > Considering DESTINT, maybe empty string is better...
> > > 
> > > By the way, what's the first parameter plat_specific? In all platforms I 
> > > have, it doesn't make any difference...
> > Technically, it's for arch-dependent vs arch-independent modules, i.e. 
> > should be True for .so extensions and False for .py modules.
> > 
> > Judging by the documentation, it's only used to switch between 
> > `sys.base_prefix` (i.e. `--prefix` given to build Python) and 
> > `sys.base_exec_prefix` (`--exec-prefix`). However, I'm not aware of any 
> > platform where two different prefixes are used for Python.
> > 
> > When a prefix is given as third argument, its value is ignored. So True vs 
> > False shouldn't really matter here, hence I've left it at the default 
> > (False).
> All else being equal, I would prefer the version which does not recompute the 
> relative path. However, if there is a meaningful functional difference 
> between the two versions, then we can stick to this one, if it is more 
> correct. As for whether the difference is "meaningful" and which version is 
> more "correct", you guys are probably more qualified to answer that than I 
> am...
I'll switch to the no recompute version in this patch. While they have some 
difference, I'm not aware of any platform where that matters. So I'll go with 
the easier way.

Also it is weird to build differently based on install prefix. Specially the 
prefix can be overwritten by DESTDIR afterwards..



Comment at: lldb/scripts/finishSwigWrapperClasses.py:196
+  "--useSystemSix": "o",
+  "--lldbPythonPath": "m"}
 

labath wrote:
> Given that the arg is marked as mandatory here, is there a need for the check 
> in `get_framework_python_dir`? Maybe that could be an assert ?
Other mandatory parameters do not even have an assert... Remove the check in 
`get_framework_python_dir`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-07 Thread Pavel Labath via Phabricator via lldb-commits

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

Thanks for the cleanup. This looks fine to me. I'll leave it up to you to 
figure out the best way to compute the relative python path...




Comment at: lldb/CMakeLists.txt:42
+COMMAND ${PYTHON_EXECUTABLE}
+-c "import distutils.sysconfig, sys; 
print(distutils.sysconfig.get_python_lib(True, False, sys.argv[1]))"
+${CMAKE_INSTALL_PREFIX}

mgorny wrote:
> hhb wrote:
> > hhb wrote:
> > > mgorny wrote:
> > > > I still like my `(False, False, '')` version better than having to 
> > > > recalculate path afterwards.
> > > I don't have a strong opinion here. Let's see what labath@ think.
> > > 
> > > That been said, I did this because some distribution modified 
> > > get_python_lib() to return differently based on prefix. One example is 
> > > Debian/Ubuntu, where 'dist-packages' will be used if prefix is '', '/usr' 
> > > or '/usr/local'.
> > > 
> > > In reality, that only makes difference when CMAKE_INSTALL_PREFIX is set. 
> > > But I guess it doesn't really matter whether we use 'dist-packages' or 
> > > 'site-packages' that time, as long as it is consistent everywhere.
> > Considering DESTINT, maybe empty string is better...
> > 
> > By the way, what's the first parameter plat_specific? In all platforms I 
> > have, it doesn't make any difference...
> Technically, it's for arch-dependent vs arch-independent modules, i.e. should 
> be True for .so extensions and False for .py modules.
> 
> Judging by the documentation, it's only used to switch between 
> `sys.base_prefix` (i.e. `--prefix` given to build Python) and 
> `sys.base_exec_prefix` (`--exec-prefix`). However, I'm not aware of any 
> platform where two different prefixes are used for Python.
> 
> When a prefix is given as third argument, its value is ignored. So True vs 
> False shouldn't really matter here, hence I've left it at the default (False).
All else being equal, I would prefer the version which does not recompute the 
relative path. However, if there is a meaningful functional difference between 
the two versions, then we can stick to this one, if it is more correct. As for 
whether the difference is "meaningful" and which version is more "correct", you 
guys are probably more qualified to answer that than I am...



Comment at: lldb/scripts/finishSwigWrapperClasses.py:196
+  "--useSystemSix": "o",
+  "--lldbPythonPath": "m"}
 

Given that the arg is marked as mandatory here, is there a need for the check 
in `get_framework_python_dir`? Maybe that could be an assert ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-05 Thread Michał Górny via Phabricator via lldb-commits

mgorny added inline comments.



Comment at: lldb/CMakeLists.txt:42
+COMMAND ${PYTHON_EXECUTABLE}
+-c "import distutils.sysconfig, sys; 
print(distutils.sysconfig.get_python_lib(True, False, sys.argv[1]))"
+${CMAKE_INSTALL_PREFIX}

hhb wrote:
> hhb wrote:
> > mgorny wrote:
> > > I still like my `(False, False, '')` version better than having to 
> > > recalculate path afterwards.
> > I don't have a strong opinion here. Let's see what labath@ think.
> > 
> > That been said, I did this because some distribution modified 
> > get_python_lib() to return differently based on prefix. One example is 
> > Debian/Ubuntu, where 'dist-packages' will be used if prefix is '', '/usr' 
> > or '/usr/local'.
> > 
> > In reality, that only makes difference when CMAKE_INSTALL_PREFIX is set. 
> > But I guess it doesn't really matter whether we use 'dist-packages' or 
> > 'site-packages' that time, as long as it is consistent everywhere.
> Considering DESTINT, maybe empty string is better...
> 
> By the way, what's the first parameter plat_specific? In all platforms I 
> have, it doesn't make any difference...
Technically, it's for arch-dependent vs arch-independent modules, i.e. should 
be True for .so extensions and False for .py modules.

Judging by the documentation, it's only used to switch between 
`sys.base_prefix` (i.e. `--prefix` given to build Python) and 
`sys.base_exec_prefix` (`--exec-prefix`). However, I'm not aware of any 
platform where two different prefixes are used for Python.

When a prefix is given as third argument, its value is ignored. So True vs 
False shouldn't really matter here, hence I've left it at the default (False).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-05 Thread Haibo Huang via Phabricator via lldb-commits

hhb marked an inline comment as not done.
hhb added inline comments.



Comment at: lldb/CMakeLists.txt:42
+COMMAND ${PYTHON_EXECUTABLE}
+-c "import distutils.sysconfig, sys; 
print(distutils.sysconfig.get_python_lib(True, False, sys.argv[1]))"
+${CMAKE_INSTALL_PREFIX}

hhb wrote:
> mgorny wrote:
> > I still like my `(False, False, '')` version better than having to 
> > recalculate path afterwards.
> I don't have a strong opinion here. Let's see what labath@ think.
> 
> That been said, I did this because some distribution modified 
> get_python_lib() to return differently based on prefix. One example is 
> Debian/Ubuntu, where 'dist-packages' will be used if prefix is '', '/usr' or 
> '/usr/local'.
> 
> In reality, that only makes difference when CMAKE_INSTALL_PREFIX is set. But 
> I guess it doesn't really matter whether we use 'dist-packages' or 
> 'site-packages' that time, as long as it is consistent everywhere.
Considering DESTINT, maybe empty string is better...

By the way, what's the first parameter plat_specific? In all platforms I have, 
it doesn't make any difference...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-05 Thread Haibo Huang via Phabricator via lldb-commits

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

Comment at: lldb/CMakeLists.txt:42
+COMMAND ${PYTHON_EXECUTABLE}
+-c "import distutils.sysconfig, sys; 
print(distutils.sysconfig.get_python_lib(True, False, sys.argv[1]))"
+${CMAKE_INSTALL_PREFIX}

mgorny wrote:
> I still like my `(False, False, '')` version better than having to 
> recalculate path afterwards.
I don't have a strong opinion here. Let's see what labath@ think.

That been said, I did this because some distribution modified get_python_lib() 
to return differently based on prefix. One example is Debian/Ubuntu, where 
'dist-packages' will be used if prefix is '', '/usr' or '/usr/local'.

In reality, that only makes difference when CMAKE_INSTALL_PREFIX is set. But I 
guess it doesn't really matter whether we use 'dist-packages' or 
'site-packages' that time, as long as it is consistent everywhere.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442

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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-05 Thread Michał Górny via Phabricator via lldb-commits

mgorny added inline comments.



Comment at: lldb/CMakeLists.txt:42
+COMMAND ${PYTHON_EXECUTABLE}
+-c "import distutils.sysconfig, sys; 
print(distutils.sysconfig.get_python_lib(True, False, sys.argv[1]))"
+${CMAKE_INSTALL_PREFIX}

I still like my `(False, False, '')` version better than having to recalculate 
path afterwards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442



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

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-04 Thread Haibo Huang via Phabricator via lldb-commits

hhb updated this revision to Diff 223346.
hhb added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442

Files:
  lldb/CMakeLists.txt
  lldb/scripts/CMakeLists.txt
  lldb/scripts/Python/finishSwigPythonLLDB.py
  lldb/scripts/finishSwigWrapperClasses.py
  lldb/scripts/get_relative_lib_dir.py
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -48,8 +48,7 @@
 
 protected:
   static void ComputePythonDirForApple(llvm::SmallVectorImpl );
-  static void ComputePythonDirForPosix(llvm::SmallVectorImpl );
-  static void ComputePythonDirForWindows(llvm::SmallVectorImpl );
+  static void ComputePythonDir(llvm::SmallVectorImpl );
 };
 } // namespace lldb_private
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -305,39 +305,20 @@
   auto rend = llvm::sys::path::rend(path_ref);
   auto framework = std::find(rbegin, rend, "LLDB.framework");
   if (framework == rend) {
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 return;
   }
   path.resize(framework - rend);
   llvm::sys::path::append(path, style, "LLDB.framework", "Resources", "Python");
 }
 
-void ScriptInterpreterPython::ComputePythonDirForPosix(
+void ScriptInterpreterPython::ComputePythonDir(
 llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::posix;
-#if defined(LLDB_PYTHON_RELATIVE_LIBDIR)
   // Build the path by backing out of the lib dir, then building with whatever
   // the real python interpreter uses.  (e.g. lib for most, lib64 on RHEL
-  // x86_64).
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, LLDB_PYTHON_RELATIVE_LIBDIR);
-#else
-  llvm::sys::path::append(path, style,
-  "python" + llvm::Twine(PY_MAJOR_VERSION) + "." +
-  llvm::Twine(PY_MINOR_VERSION),
-  "site-packages");
-#endif
-}
-
-void ScriptInterpreterPython::ComputePythonDirForWindows(
-llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::windows;
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, "lib", "site-packages");
-
-  // This will be injected directly through FileSpec.GetDirectory().SetString(),
-  // so we need to normalize manually.
-  std::replace(path.begin(), path.end(), '\\', '/');
+  // x86_64, or bin on Windows).
+  llvm::sys::path::remove_filename(path);
+  llvm::sys::path::append(path, LLDB_PYTHON_RELATIVE_LIBDIR);
 }
 
 FileSpec ScriptInterpreterPython::GetPythonDir() {
@@ -350,11 +331,10 @@
 
 #if defined(__APPLE__)
 ComputePythonDirForApple(path);
-#elif defined(_WIN32)
-ComputePythonDirForWindows(path);
 #else
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 #endif
+llvm::sys::path::native(path);
 spec.GetDirectory().SetString(path);
 return spec;
   }();
Index: lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
@@ -1,15 +1,7 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
-  # Call a python script to gather the arch-specific libdir for
-  # modules like the lldb module.
-  execute_process(
-COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../../../../scripts/get_relative_lib_dir.py
-RESULT_VARIABLE get_libdir_status
-OUTPUT_VARIABLE relative_libdir
-)
-  if (get_libdir_status EQUAL 0)
-add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${relative_libdir}")
-  endif()
+if(NOT LLDB_PYTHON_RELATIVE_PATH)
+  message(FATAL_ERROR "LLDB_PYTHON_RELATIVE_PATH is not set.")
 endif()
+add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${LLDB_PYTHON_RELATIVE_PATH}")
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
Index: lldb/scripts/get_relative_lib_dir.py
===
--- lldb/scripts/get_relative_lib_dir.py
+++ /dev/null
@@ -1,44 +0,0 @@
-import distutils.sysconfig
-import os
-import platform
-import re
-import sys
-
-
-def get_python_relative_libdir():
-"""Returns the appropropriate python libdir

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

2019-10-03 Thread Haibo Huang via Phabricator via lldb-commits

hhb updated this revision to Diff 223135.
hhb added a comment.

Fix description..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68442

Files:
  lldb/CMakeLists.txt
  lldb/scripts/CMakeLists.txt
  lldb/scripts/Python/finishSwigPythonLLDB.py
  lldb/scripts/finishSwigWrapperClasses.py
  lldb/scripts/get_relative_lib_dir.py
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -48,8 +48,7 @@
 
 protected:
   static void ComputePythonDirForApple(llvm::SmallVectorImpl );
-  static void ComputePythonDirForPosix(llvm::SmallVectorImpl );
-  static void ComputePythonDirForWindows(llvm::SmallVectorImpl );
+  static void ComputePythonDir(llvm::SmallVectorImpl );
 };
 } // namespace lldb_private
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -305,39 +305,20 @@
   auto rend = llvm::sys::path::rend(path_ref);
   auto framework = std::find(rbegin, rend, "LLDB.framework");
   if (framework == rend) {
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 return;
   }
   path.resize(framework - rend);
   llvm::sys::path::append(path, style, "LLDB.framework", "Resources", "Python");
 }
 
-void ScriptInterpreterPython::ComputePythonDirForPosix(
+void ScriptInterpreterPython::ComputePythonDir(
 llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::posix;
-#if defined(LLDB_PYTHON_RELATIVE_LIBDIR)
   // Build the path by backing out of the lib dir, then building with whatever
   // the real python interpreter uses.  (e.g. lib for most, lib64 on RHEL
-  // x86_64).
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, LLDB_PYTHON_RELATIVE_LIBDIR);
-#else
-  llvm::sys::path::append(path, style,
-  "python" + llvm::Twine(PY_MAJOR_VERSION) + "." +
-  llvm::Twine(PY_MINOR_VERSION),
-  "site-packages");
-#endif
-}
-
-void ScriptInterpreterPython::ComputePythonDirForWindows(
-llvm::SmallVectorImpl ) {
-  auto style = llvm::sys::path::Style::windows;
-  llvm::sys::path::remove_filename(path, style);
-  llvm::sys::path::append(path, style, "lib", "site-packages");
-
-  // This will be injected directly through FileSpec.GetDirectory().SetString(),
-  // so we need to normalize manually.
-  std::replace(path.begin(), path.end(), '\\', '/');
+  // x86_64, or bin on Windows).
+  llvm::sys::path::remove_filename(path);
+  llvm::sys::path::append(path, LLDB_PYTHON_RELATIVE_LIBDIR);
 }
 
 FileSpec ScriptInterpreterPython::GetPythonDir() {
@@ -350,11 +331,10 @@
 
 #if defined(__APPLE__)
 ComputePythonDirForApple(path);
-#elif defined(_WIN32)
-ComputePythonDirForWindows(path);
 #else
-ComputePythonDirForPosix(path);
+ComputePythonDir(path);
 #endif
+llvm::sys::path::native(path);
 spec.GetDirectory().SetString(path);
 return spec;
   }();
Index: lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
@@ -1,15 +1,7 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
-  # Call a python script to gather the arch-specific libdir for
-  # modules like the lldb module.
-  execute_process(
-COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../../../../scripts/get_relative_lib_dir.py
-RESULT_VARIABLE get_libdir_status
-OUTPUT_VARIABLE relative_libdir
-)
-  if (get_libdir_status EQUAL 0)
-add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${relative_libdir}")
-  endif()
+if(NOT LLDB_PYTHON_RELATIVE_PATH)
+  message(FATAL_ERROR "LLDB_PYTHON_RELATIVE_PATH is not set yet.")
 endif()
+add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${LLDB_PYTHON_RELATIVE_PATH}")
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
Index: lldb/scripts/get_relative_lib_dir.py
===
--- lldb/scripts/get_relative_lib_dir.py
+++ /dev/null
@@ -1,44 +0,0 @@
-import distutils.sysconfig
-import os
-import platform
-import re
-import sys
-
-
-def get_python_relative_libdir():
-"""Returns the appropropriate python

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

[Lldb-commits] [PATCH] D68442: [lldb] Unifying lldb python path

14 matches

Site Navigation

Mail list logo

Footer information