Page MenuHomePhabricator

[CMake] Some cleanup around test preparations
AbandonedPublic

Authored by sgraenitz on Jan 7 2019, 11:02 AM.

Details

Summary

Fix some issues in the code around D56399:

  • remove outdated comments
  • streamline empty-string conditions
  • remove 2 uses of LLDB_TEST_USE_CUSTOM_C(XX)_COMPILER -- there are two more in lit/CMakeLists.txt that should follow
  • turn FATAL_ERROR no test compilers into WARNING (plus add quotes around uses in lit/CMakeLists.txt)

Event Timeline

sgraenitz created this revision.Jan 7 2019, 11:02 AM
sgraenitz marked an inline comment as done.Jan 7 2019, 11:09 AM
sgraenitz added inline comments.
CMakeLists.txt
45

I think these would be all good to fix, but I am not sure I can do it now. Should we keep remaining ones as FIXME?
Currently investigating the LLVM_BINARY_DIR issue.

sgraenitz marked an inline comment as done.Jan 7 2019, 11:31 AM
sgraenitz added inline comments.
CMakeLists.txt
42

This was here already when the file was created:
https://github.com/llvm-mirror/lldb/blob/98c187f2613fd8e2f5cdf5f8f35503a102a54f11/cmake/modules/LLDBStandalone.cmake#L67

Apparently the below default paths for dsymutil, FileCheck and clang never worked in standalone builds..

JDevlieghere added inline comments.Jan 7 2019, 2:48 PM
CMakeLists.txt
41

Why is this unused? Are we only using LLDB_TEST_C_COMPILER and if so, why?

42

Instead of this FIXME, should we explicitly unset this variable to prevent mistakes in the future? Or is this fixable?

This looks fine to me. I recommend including Stella in changes relating to this, as I recall some subtleties here wrt. multi-config (e.g., MSVC) generators.

CMakeLists.txt
41

I think that's because this only gets passed to dotest, that it has it's own logic for deducing the C++ compiler name.

sgraenitz added inline comments.Jan 8 2019, 2:48 AM
CMakeLists.txt
41

Yes, we only ever pass on LLDB_TEST_C_COMPILER and it should be sufficient.
https://github.com/llvm-mirror/lldb/blob/2b03c2512ebf1999e330e85d60b7f93703483485/test/CMakeLists.txt#L53

https://lldb.llvm.org/build.html reads: Note that MSVC is not supported here, it must be a path to a clang executable.
As clang == clang++ ⇒ q.e.d :)

I think what we really want here is a LLDB_TEST_COMPILER setting, but this means: update docs and build bots, potentially phase out the setting? I am not sure about their popularity.

42

Good point. Not sure what the consequences would be. I will try and see what happens.

sgraenitz updated this revision to Diff 180643.Jan 8 2019, 5:17 AM

Act on FIXME's in subsequent commits and remove comments here

sgraenitz marked 7 inline comments as done.Jan 8 2019, 8:31 AM
sgraenitz added inline comments.
CMakeLists.txt
41

There was a previous initiative for cleanup in D39215. My follow-up patch D56440 aims to finish this.

42

See followup patch D56443.

stella.stamenova requested changes to this revision.Jan 10 2019, 9:15 AM
stella.stamenova added inline comments.
CMakeLists.txt
49

Nit: it looks like your if statements lack a space before the parenthesis, while all the existing ones have a space. Please be consistent with the existing convention.

54

You are making a significant change to the compiler selection logic in your second change. I think it would be better to not make any changes here in order to have all of them in a single change.

This revision now requires changes to proceed.Jan 10 2019, 9:15 AM
sgraenitz abandoned this revision.Jan 11 2019, 9:31 AM

I will split off the "dead code elimination" part and close both reviews in favor of a more comprehensive fix.

CMakeLists.txt
49

That doesn't seem to be the most popular convention ;-)
BTW do we have documentation for CMake coding style?