[Lldb-commits] [PATCH] D132257: [lldb] Create flag to use a custom libcxx in tests

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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