Page MenuHomePhabricator

[lldb/cmake] Enable more verbose find_package output.
ClosedPublic

Authored by mattd on Feb 20 2020, 11:15 AM.

Details

Summary

The purpose of this patch is to make identifying missing dependencies clearer to the user.
find_package will report if a package is not found, that output, combined with the exiting
status message, is clearer than not having the additional verbosity.

If the SWIG dependency is required {LLDB_ENABLE_PYTHON, LLDB_ENABLE_LUA}
and SWIG is not available, fail the configuration step. Terminate the
configure early rather than later with a clear error message.

We could possibly modify:
llvm-project/lldb/cmake/modules/FindPythonInterpAndLibs.cmake
However, the patch here seems clear in my opinion.

Diff Detail

Event Timeline

mattd created this revision.Feb 20 2020, 11:15 AM
jrm added a subscriber: jrm.Feb 20 2020, 11:18 AM
JDevlieghere requested changes to this revision.Feb 20 2020, 3:57 PM

It should never be possible for LLDB_ENABLE_PYTHON to be true but SWIG_FOUND to be false. The modules FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake should fail early when SWIG isn't found.

This revision now requires changes to proceed.Feb 20 2020, 3:57 PM
mattd added a comment.Feb 20 2020, 4:28 PM

It should never be possible for LLDB_ENABLE_PYTHON to be true but SWIG_FOUND to be false. The modules FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake should fail early when SWIG isn't found.

Thanks for taking a look. Would you be okay with making the status message() in FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake fatal, that way the dependency error is clearer?

mattd updated this revision to Diff 245763.Feb 20 2020, 4:41 PM
mattd retitled this revision from [lldb/bindings] Fail configuration when a required dependency is not met. to [lldb/cmake] Fail configuration if a required swig dependency is not met..
JDevlieghere added a comment.EditedFeb 20 2020, 4:42 PM

It should never be possible for LLDB_ENABLE_PYTHON to be true but SWIG_FOUND to be false. The modules FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake should fail early when SWIG isn't found.

Thanks for taking a look. Would you be okay with making the status message() in FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake fatal, that way the dependency error is clearer?

That would defeat the purpose of auto-detecting these dependencies. Please take a look at D71306 for all the details. The summary is that all optional dependencies default to Auto: where we enable them if we can find them. You can override this behavior by passing LLDB_ENABLE_PYTHON=ON to CMake, in which case not finding Python (or SWIG) will be a fatal error.

I agree that it can be confusing to figure out that Python got disabled because SWIG wasn't found. Currently we call find_package with QUIET from FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake. I think we should remove that so that the user gets more information in CMake's configuration output. Would that address your concerns?

mattd added a comment.Feb 20 2020, 5:04 PM

It should never be possible for LLDB_ENABLE_PYTHON to be true but SWIG_FOUND to be false. The modules FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake should fail early when SWIG isn't found.

Thanks for taking a look. Would you be okay with making the status message() in FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake fatal, that way the dependency error is clearer?

That would defeat the purpose of auto-detecting these dependencies. Please take a look at D71306 for all the details. The summary is that all optional dependencies default to Auto: where we enable them if we can find them. You can override this behavior by passing LLDB_ENABLE_PYTHON=ON to CMake, in which case not finding Python (or SWIG) will be a fatal error.

Thanks for the clarification there.

I agree that it can be confusing to figure out that Python got disabled because SWIG wasn't found. Currently we call find_package with QUIET from FindPythonInterpAndLibs.cmake and FindLuaAndSwig.cmake. I think we should remove that so that the user gets more information in CMake's configuration output. Would that address your concerns?

Ah, yes, dropping the QUIET produces meaningful insight as to what might be missing. I think others, who might not have all the dependencies, might find this level of verbosity clearer, so that they can more quickly understand what is missing in their build environment.

mattd updated this revision to Diff 245771.Feb 20 2020, 5:10 PM
mattd retitled this revision from [lldb/cmake] Fail configuration if a required swig dependency is not met. to [lldb/cmake] Enable more verbose find_package output..
mattd edited the summary of this revision. (Show Details)
JDevlieghere accepted this revision.Feb 20 2020, 8:01 PM

LGMT. Thank you!

This revision is now accepted and ready to land.Feb 20 2020, 8:01 PM
This revision was automatically updated to reflect the committed changes.