[PATCH] D61660: [cmake] Make google benchmark project call llvm_add_library
Don Hinton via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 8 09:49:24 PDT 2019
hintonda added a comment.
In D61660#1495366 <https://reviews.llvm.org/D61660#1495366>, @lebedev.ri wrote:
> In D61660#1495354 <https://reviews.llvm.org/D61660#1495354>, @hintonda wrote:
>
> > In D61660#1495331 <https://reviews.llvm.org/D61660#1495331>, @lebedev.ri wrote:
> >
> > > .. this doesn't look like what i proposed, and has some unrelated diff?
> >
> >
> > It uses your wrapper suggestion, but keeps everything local to benchmark. Generally, if projects are build within other projects, e.g., see `clang/CMakeLists.txt`, they don't redeclare `project()`. Doing so wipes out some state, most notably policies. By checking, you preserve whatever the parent project set.
>
>
> I'm sorry, i don't understand.
> Why?
> The diff i proposed i will happily merge upstream into google benchmark.
> This - no.
Sure, but so will this one, and it doesn't require any changes to LLVM.
Alternatively, I could do one of the following:
- leave the `project(benchmark)` command as is and move the conditional before it.
- fix benchmark to build libraries correctly on OSX -- essentially duplicating logic in `llvm_setup_rpath()`.
But I really don't think fixing broken sub-projects should require changes to LLVM. It's the subproject that's broken, not LLVM.
>
>
>> The only other change is just to fix the path to be relative to the cmake file instead of the project root, which will be different when contained in another project.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61660/new/
https://reviews.llvm.org/D61660
More information about the llvm-commits
mailing list