Page MenuHomePhabricator

[cmake] Do not generate benchmark targets by default
AbandonedPublic

Authored by hintonda on May 11 2019, 7:13 PM.

Details

Summary

Since the benchmark subproject doesn't depend on llvm,
there's not reason to generate its targets by default, i.e., there are
no dependencies and changes to llvm can't break it.

This change sets the default value for LLVM_INCLUDE_BENCHMARKS =
LLVM_BUILD_BENCHMARKS so that the targets are only generated by
default when they are actually being built.

Users can still include the targets by passing
LLVM_INCLUDE_BENCHMARKS=ON from the command line.

Event Timeline

hintonda created this revision.May 11 2019, 7:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2019, 7:13 PM
Herald added a subscriber: mgorny. · View Herald Transcript
kbobyrev edited reviewers, added: ilya-biryukov, sammccall; removed: kbobyrev.May 12 2019, 12:32 PM

Hi @hintonda!

Could you please elaborate a bit on why you would like to propose this change? I couldn't understand the argument here:

Since the benchmark subproject doesn't depend on llvm, there's not reason to generate its targets by default, i.e., there are no dependencies and changes to llvm can't break it.

From my perspective, generating targets for benchmarks seems fine since it doesn't add it to all anyway and it is easier to build benchmarks. I would be interested to learn why you think this behavior is inconvenient.

Also

  • If LLVM_INCLUDE_BENCHMARKS is set to LLVM_BUILD_BENCHMARKS then there's probably no reason in two variables.
  • Changing the default value of CMake variable should be reflected in the LLVM's CMake docs since it explicitly mentions default value.

I might not have any time to review the patch soon, though :( I'm hoping that either Ilya or Sam can help with this change because we were discussing it before landing the original patch.

Hi @hintonda!

Could you please elaborate a bit on why you would like to propose this change? I couldn't understand the argument here:

Since the benchmark subproject doesn't depend on llvm, there's not reason to generate its targets by default, i.e., there are no dependencies and changes to llvm can't break it.

Why would I want to generate targets for a 3rd party feature I don't use? Especially one that issues warnings.

From my perspective, generating targets for benchmarks seems fine since it doesn't add it to all anyway and it is easier to build benchmarks. I would be interested to learn why you think this behavior is inconvenient.

It was added as an "opt-out" feature, so as soon as it landed, I started getting RPATH warnings on Darwin, even though I hadn't changed anything. Had it been done as an "opt-in" feature, only people who wanted it would have been affected.

I've submitted a separate patch for the RPATH issue, but in reality, there's no reason for cmake to do the extra work, and spend extra time, generating targets for a feature I'm not using.

Also

  • If LLVM_INCLUDE_BENCHMARKS is set to LLVM_BUILD_BENCHMARKS then there's probably no reason in two variables.
  • Changing the default value of CMake variable should be reflected in the LLVM's CMake docs since it explicitly mentions default value.

    I might not have any time to review the patch soon, though :( I'm hoping that either Ilya or Sam can help with this change because we were discussing it before landing the original patch.

With this patch, the user can "opt-in" to either generate benchmark targets, or build them by setting only on variable: the include version to get targets or the build version to both generate targets and build them. Anyone who is setting the build variable won't see any difference.

hintonda abandoned this revision.May 24 2019, 4:38 PM

I'll just pass LLVM_INCLUDE_BENCHMARK=OFF until this library is fixed to configure properly on Mac.