[Lldb-commits] [PATCH] D66327: [dotest] Make it possible to forward CCC_OVERRIDE_OPTIONS to Make

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

In D66327#1632684 <https://reviews.llvm.org/D66327#1632684>, @labath wrote:

> How do you envision this to be used? Via something like `CCC_OVERRIDE_OPTIONS=foo ninja check-lldb` ?
>
> Overall, I think it would be nice to avoid the tests being affected by the inherited environment variables because that opens the door for tests to unexpectedly fail or simply behave differently just because someone happens to have some weird environment variable set (and he may not even know about it). I think this is also consistent with the aproach taken in llvm (see e.g. `possibly_dangerous_env_vars` in `llvm/utils/lit/lit/llvm/config.py`). Overall, being able to run the (dotest) test suite with different options would be nice, but I think it would be better if there was a more explicit way to do that (e.g., a command line argument). Incidentally, `dotest` already has a command line argument (`--env`), which seems to be exactly intended for its purpose. Couldn't you just use that?


I considered this, and the reason I didn't go with that approach is because of how the builder gets it arguments: they're already passed through the environment. Given that `CCC_OVERRIDE_OPTIONS` is special in that it is consumed through the environment, I felt like this warranted just forwarding it.

I think passing the `--env` flags to the builder would be good, but it's going to be messy unless we move away from passing everything to the builder in the environment.

> (Also, instead of CCC_OVERRIDE_OPTIONS, I think I'd try to use CFLAGS_EXTRAS, because that explicitly handled by makefiles, and so it gives an easier way for tests to opt-out of this when they want very specific flags for something)

They're not exactly the same, `CCC_OVERRIDE_OPTIONS` allows you to do cool stuff like `'s/-g(lldb)?$/-gdwarf-5/'`. I want to use it on our matrix bot to check against different DWARF versions. The old bot was creating a wrapper around clang that set the `CCC_OVERRIDE_OPTIONS` and changes out the test compiler, but this would be a lot easier.



================
Comment at: lldb/packages/Python/lldbsuite/test/plugins/builder_base.py:39
 
+def getEnv():
+    """Returns the build environment."""
----------------
labath wrote:
> I am confused. How is this function used and what's its relation the the second change below?
I uploaded a slightly outdated patch, the function below should call `getEnv`. 


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66327