Page MenuHomePhabricator

Don't install example analyzer plugins
ClosedPublic

Authored by aaronpuchert on Sep 27 2019, 5:25 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

aaronpuchert created this revision.Sep 27 2019, 5:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2019, 5:25 PM

+@Szelethus because i'm a bit out-of-the-loop on plugins: i very vaguely remember that we decided to put them into tests(?)

In D68172#1686772, @NoQ wrote:

+@Szelethus because i'm a bit out-of-the-loop on plugins: i very vaguely remember that we decided to put them into tests(?)

The plugins were in test, but D62445 changed that. Another solution would have been to move them into the unittests directory. But other example plugins are also part of the ordinary source tree, see for example llvm/lib/Transforms/Hello. In fact this change is based on LLVMHello, which also has BUILDTREE_ONLY.

aaronpuchert edited the summary of this revision. (Show Details)Sep 28 2019, 4:02 AM

The patch looks alright, I won't formally accept because if I knew how these worked, it wouldn't have caused so much pain to so many people :)

In D68172#1686772, @NoQ wrote:

+@Szelethus because i'm a bit out-of-the-loop on plugins: i very vaguely remember that we decided to put them into tests(?)

The plugins were in test, but D62445 changed that. Another solution would have been to move them into the unittests directory. But other example plugins are also part of the ordinary source tree, see for example llvm/lib/Transforms/Hello. In fact this change is based on LLVMHello, which also has BUILDTREE_ONLY.

I honestly feel bad about these plugins. The first analyzer plugin laid in the examples/ folder, but I was cautioned against adding more there as we're not really supporting them, but I think all the trouble of moving them wasn't worth it at all for such a negligible "gain".

The patch looks alright, I won't formally accept because if I knew how these worked, it wouldn't have caused so much pain to so many people :)

If it helps, this is from add_llvm_library in llvm/cmake/modules/AddLLVM.cmake:

if (ARG_MODULE AND NOT TARGET ${name})
  # Add empty "phony" target
  add_custom_target(${name})
elseif( EXCLUDE_FROM_ALL )
  set_target_properties( ${name} PROPERTIES EXCLUDE_FROM_ALL ON)
elseif(ARG_BUILDTREE_ONLY)
  set_property(GLOBAL APPEND PROPERTY LLVM_EXPORTS_BUILDTREE_ONLY ${name})
else()
  # ... installation code ... 
  set_property(GLOBAL APPEND PROPERTY LLVM_EXPORTS ${name})
endif()

The plugins are modules, but the targets exist, so we don't run into the first branch. With EXCLUDE_FROM_ALL we would make the target not being build by default, and with ARG_BUILDTREE_ONLY we make them build, but not install.

I honestly feel bad about these plugins. The first analyzer plugin laid in the examples/ folder, but I was cautioned against adding more there as we're not really supporting them, but I think all the trouble of moving them wasn't worth it at all for such a negligible "gain".

Moving them out of test was probably necessary, but the current location should be fine.

lebedev.ri accepted this revision.Sep 28 2019, 5:53 AM

I feel like i can stamp this.
There's similar-isn problem with lit-cpuid which is referenced in LLVMExports.cmake but is packaged in debian in lldb...

This revision is now accepted and ready to land.Sep 28 2019, 5:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2019, 6:27 AM