Page MenuHomePhabricator

[cmake] Fix clang builds with BUILD_SHARED=ON and CLANG_LINK_CLANG_DYLIB=ON
ClosedPublic

Authored by tstellar on Oct 4 2019, 3:27 PM.

Diff Detail

Event Timeline

tstellar created this revision.Oct 4 2019, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 3:27 PM
Herald added a subscriber: mgorny. · View Herald Transcript

Got me a while to understand what you're saying. It's all high magic but looks reasonable enough.

After thinking about this more, do we want to even try to support the BUILD_SHARED=ON + CLANG_LINK_CLANG_DYLIB=ON configuration? We don't support this in llvm.

After thinking about this more, do we want to even try to support the BUILD_SHARED=ON + CLANG_LINK_CLANG_DYLIB=ON configuration? We don't support this in llvm.

Actually, even if we don't support this in clang, we still need to fix this. Otherwise libclang-cpp.so will not be useable at all when clang has been built with BUILD_SHARED=ON .

Ping. This bug is preventing upstream projects from using libclang-cpp.so, since building with -DENABLE_SHARED=ON is still common.

mgorny accepted this revision.Jan 21 2020, 9:22 PM

Well, it looks correct to me. Not sure if it's the best approach but it can be refined in the future.

This revision is now accepted and ready to land.Jan 21 2020, 9:22 PM
beanz accepted this revision.Jan 22 2020, 8:26 AM

Seems reasonable.

This revision was automatically updated to reflect the committed changes.
tstellar reopened this revision.Jan 22 2020, 6:49 PM
This revision is now accepted and ready to land.Jan 22 2020, 6:49 PM
tstellar updated this revision to Diff 239764.Jan 22 2020, 6:51 PM

Rewrite patch to use cmake features available with cmake >= 3.4. The previous
version used a 3.6 feature: list(FILTER ...)

Unit tests: pass. 62114 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

mgorny added inline comments.Jan 22 2020, 10:02 PM
clang/tools/clang-shlib/CMakeLists.txt
28

Any reason not to use if(${lib} MATCHES ...)?

https://cmake.org/cmake/help/v3.0/command/if.html

Not sure if ${} is needed, I get always confused by that.

tstellar updated this revision to Diff 239919.Jan 23 2020, 8:52 AM
tstellar marked 2 inline comments as done.

Address review comments

tstellar added inline comments.Jan 23 2020, 8:57 AM
clang/tools/clang-shlib/CMakeLists.txt
28

Thanks for the tip, I updated the patch to use this.

tstellar requested review of this revision.Jan 31 2020, 4:09 PM

Ping.

mgorny accepted this revision.Jan 31 2020, 10:28 PM

Still LGTM.

This revision is now accepted and ready to land.Jan 31 2020, 10:28 PM
This revision was automatically updated to reflect the committed changes.