fdeazeve added inline comments.
================
Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:338-339
+ if args.custom_libcpp:
+ os.environ['CUSTOM_LIBCPP'] = args.custom_libcpp
+
----------------
aprantl wrote:
> JDevlieghere wrote:
> > aprantl wrote:
> > > JDevlieghere wrote:
> > > > Please don't rely on environment variables to pass arguments to the
> > > > Make invocation. This makes it really tedious to debug make
> > > > invocations. Instead, pass these explicitly as part of the make
> > > > invocation from the builders
> > > > (`packages/Python/lldbsuite/test/builders/`).
> > > That is a very good point. Maybe we should just fix the -E option, which
> > > doesn't work anyway due to Makefiles setting CFLAGS_EXTRAS and use that.
> > I think a specific flag is fine, we already have something similar for
> > using the "hermetic" libc++ (I left a comment below about that). We should
> > see if we can unify this.
> It looks like (apart from making -E not use CFLAGS_EXTRAS but a fresh
> variable) we should use that mechanism for all the options passed using
> os.environ().
I'm a bit puzzled by the Builder class, since I don't recall encountering it
while tracing the variables from CMake to the final Makefile invocation. I'll
try to figure this out based on your patch.
================
Comment at: lldb/packages/Python/lldbsuite/test/dotest_args.py:175
help='A plugin whose tests will be enabled. The only currently
supported plugin is intel-pt.')
+ group.add_argument(
+ '--custom-libcpp',
----------------
JDevlieghere wrote:
> We should also think about how this interacts with `--hermetic-libcxx` (and
> make the name of the options consistent).
If I understand correctly, that flag is a more restricted behaviour of what is
being implemented here: `hermetic-libcxx` is _only_ about looking for a libcxx
built alongside LLDB / Clang, whereas this patch allows an arbitrary libcxx to
be used (the use cases we're aiming for don't even use a Clang built alongside
LLDB).
I believe `hermetic-libcxx` can be implemented in terms of the new
functionality being added here, but I'll look some more at your patch to make
sure that's the case
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132257/new/
https://reviews.llvm.org/D132257
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
