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
