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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
