This is an archive of the discontinued LLVM Phabricator instance.

[lldb/CMake] Use LLDB's autodetection logic for libxml2
ClosedPublic

Authored by JDevlieghere on Jan 6 2020, 10:11 AM.

Details

Summary

Libxml2 is already an optional dependency. It should use the same infrastructure as the other dependencies.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jan 6 2020, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 10:11 AM
Herald added a subscriber: mgorny. · View Herald Transcript
labath added inline comments.Jan 7 2020, 12:40 AM
lldb/cmake/modules/FindLibXml28.cmake
14 ↗(On Diff #236406)

Why is this under if(APPLE) ?

kwk added a subscriber: kwk.Jan 7 2020, 1:39 AM
kwk added inline comments.
lldb/cmake/modules/FindLibXml28.cmake
14 ↗(On Diff #236406)

To me this looks as if this module file FileLibXml28.cmake is only relevant for Apple. In line 10 it falls back to the standard CMake find-module file (find_package(LibXml2 QUIET)). Is this correct?

JDevlieghere marked an inline comment as done.Jan 7 2020, 9:35 AM
JDevlieghere added inline comments.
lldb/cmake/modules/FindLibXml28.cmake
14 ↗(On Diff #236406)

I'm not sure, I just kept the old behavior. I'm pretty sure it doesn't matter. Do you prefer to make it unconditional or just remove it altogether?

labath added inline comments.Jan 7 2020, 10:01 AM
lldb/cmake/modules/FindLibXml28.cmake
14 ↗(On Diff #236406)

Ah, right, I think I now see where this is coming from. I don't think it makes sense to keep this if xml is disabled then linking against it is useless. Ideally, I'd just remove it...

JDevlieghere marked 3 inline comments as done.
compnerd added inline comments.Jan 7 2020, 4:38 PM
lldb/cmake/modules/FindLibXml28.cmake
14 ↗(On Diff #236406)

Why the custom module for this? Why not use find_package(LibXml2 2.8 QUIET) and ensure that libxml2 is >= 2.8?

JDevlieghere marked an inline comment as done.Jan 7 2020, 4:49 PM
JDevlieghere added inline comments.
lldb/cmake/modules/FindLibXml28.cmake
14 ↗(On Diff #236406)

Right, with the special if(APPLE) logic gone there there's no good reason to have a custom module anymore.

labath accepted this revision.Jan 8 2020, 2:38 AM

cool

This revision is now accepted and ready to land.Jan 8 2020, 2:38 AM
This revision was automatically updated to reflect the committed changes.