Page MenuHomePhabricator

[Examples] Add add_llvm_example_library macro and use it for IR example.
AbandonedPublic

Authored by fhahn on Nov 22 2019, 3:32 AM.

Details

Summary

We need to make sure to set the right destination for the ExamplesIRTransforms
library target and respect LLVM_BUILD_EXAMPLES. This patch adds a
add_llvm_example_library macro similar to the add_llvm_example one,
which should set the destination for the install target properly and
exclude it from the ALL target if LLVM_BUILD_EXAMPLES=Off.

It should fix a problem where using LLVMExports.cmake fails, because
it expects libExampleIRTransforms.a to be installed.

Event Timeline

fhahn created this revision.Nov 22 2019, 3:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2019, 3:32 AM
Herald added a subscriber: mgorny. · View Herald Transcript
svenvh added a subscriber: svenvh.Nov 22 2019, 3:42 AM
andwar added a subscriber: andwar.Nov 22 2019, 4:00 AM
andwar added inline comments.Nov 22 2019, 4:05 AM
llvm/examples/IRTransforms/CMakeLists.txt
7

[nit] Could you add a comment explaining why this is needed? Currently it says what is being done, but that's clear from the code. This could useful to our future selves.

fhahn updated this revision to Diff 230634.Nov 22 2019, 4:38 AM

Update the patch to add a dedicated add_llvm_example_library macro, similar to add_llvm_example, which takes care of respecting LLVM_BUILD_EXAMPLES and setting the correct destinations.

fhahn marked an inline comment as done.Nov 22 2019, 4:39 AM
fhahn added inline comments.
llvm/examples/IRTransforms/CMakeLists.txt
7

Thanks for taking a look. I move the logic into a macro similar to add_llvm_example, which now takes care of respecting LLVM_BUILD_EXAMPLES and more.

fhahn retitled this revision from [Examples] Exclude ExampleIRTransforms from ALL if examples are disabled. to [Examples] Add add_llvm_example_library macro and use it for IR example..Nov 22 2019, 4:44 AM
fhahn edited the summary of this revision. (Show Details)
fhahn edited the summary of this revision. (Show Details)
compnerd requested changes to this revision.Nov 23 2019, 5:06 PM

I think that rather than adding more wrapping macros, we should fix the structure. We can mark the examples directory from ALL at the add_subdirectory level and only add the subdirectory when examples are enabled. This avoids the need for the new macro. I think that we want to move towards a more standard cmake build rather than a more specialized one.

This revision now requires changes to proceed.Nov 23 2019, 5:06 PM
fhahn added a comment.Nov 24 2019, 1:36 PM

I think that rather than adding more wrapping macros, we should fix the structure. We can mark the examples directory from ALL at the add_subdirectory level and only add the subdirectory when examples are enabled. This avoids the need for the new macro. I think that we want to move towards a more standard cmake build rather than a more specialized one.

I think sticking to more standard cmake is great. My Cmake is a bit rusty, are you suggesting to just not add the examples directory (guarding add_subdirectory with LLVM_BUILD_EXAMPLES)?

I think that would change the behavior slightly, because currently IIUC we always add the example targets, we just not build them by default and also update the target directory for installing them and it would be valuable to keep the current behavior.

w88 added a subscriber: w88.Nov 24 2019, 2:57 PM

I think that rather than adding more wrapping macros, we should fix the structure. We can mark the examples directory from ALL at the add_subdirectory level and only add the subdirectory when examples are enabled. This avoids the need for the new macro. I think that we want to move towards a more standard cmake build rather than a more specialized one.

I think sticking to more standard cmake is great. My Cmake is a bit rusty, are you suggesting to just not add the examples directory (guarding add_subdirectory with LLVM_BUILD_EXAMPLES)?

I think that would change the behavior slightly, because currently IIUC we always add the example targets, we just not build them by default and also update the target directory for installing them and it would be valuable to keep the current behavior.

Would it please be possible to have some progress on the problem?
Either this needs to land, or if it is not in an acceptable form then D69416 needs to be reverted till green.

fhahn added a comment.Dec 1 2019, 2:22 PM

Reverted as 19fd8925a4afe6efd248688cce06aceff50efe0c so we do not need to rush this fix.

fhahn added a comment.Dec 4 2019, 3:31 AM

@compnerd ping. Do you have any thoughts about my earlier response (https://reviews.llvm.org/D70590#1758209) with respect to just use add_subdirectory to control the inclusion of the examples? IIUC that would change the current behavior (example targets are added even with LLVM_BUILD_EXAMPLES=Off, they are just excluded from the ALL target and the installed stuff)

fhahn added a comment.Dec 12 2019, 7:45 AM

ping @compnerd

It would be great if you could take a look to see if my previous response makes sense to you :)

Hmm, I see, yes, that would change the behavior where we would not create the targets for it. In that case, using EXCLUDE_FROM_ALL is fine, but set it as a target property rather than passing global variables like this. But, even better would be to understand if we even need this.

Really, the install-distribution target allows the installed image to contain specific bits, and the distribution target lets you build the minimal set. Having the target being built always when you build *everything* (ALL) is the preferable option.

fhahn added a comment.Jan 3 2020, 8:58 AM

Hmm, I see, yes, that would change the behavior where we would not create the targets for it. In that case, using EXCLUDE_FROM_ALL is fine, but set it as a target property rather than passing global variables like this. But, even better would be to understand if we even need this.

Really, the install-distribution target allows the installed image to contain specific bits, and the distribution target lets you build the minimal set. Having the target being built always when you build *everything* (ALL) is the preferable option.

Thanks for taking a look. The original commit was reverted, so I'll re-commit the original change with the EXCLUDE_FROM_ALL fix. I'll check how to not pass it as global variable.

andwar added a comment.Jan 3 2020, 1:03 PM

@fhahn Since https://reviews.llvm.org/D61446 has finally landed, what about revisiting the idea of using that instead? Just a thought.

fhahn added a comment.Jan 4 2020, 8:02 AM

@fhahn Since https://reviews.llvm.org/D61446 has finally landed, what about revisiting the idea of using that instead? Just a thought.

Sure, I just re-landed the original patch, but using the new infrastructure is certainly something we should do now.

fhahn abandoned this revision.Jan 8 2020, 1:15 PM