JDevlieghere added inline comments.
================
Comment at: clang/cmake/modules/ProtobufMutator.cmake:4-5
+ set (PBM_PREFIX clang_protobuf_mutator)
+elseif(${CMAKE_CURRENT_SOURCE_DIR} MATCHES "lldb")
+ set (PBM_PREFIX lldb_protobuf_mutator)
+endif()
----------------
cassanova wrote:
> mib wrote:
> > If feels wrong to me that the clang protobuf cmake module knows about lldb.
> >
> > May be we should just have 2 separate files for clang and lldb
> My preferred solution to this was just creating a target called
> ${LLVM_VARIABLE_THAT_TELLS_YOU_THE_SUBPROJECT_NAME}_protobuf_mutator to avoid
> using if-statements and direct strings but it looks like clang and lldb
> aren't defined as subprojects unless they're being built standalone.
>
> Also in the event that some other subproject wanted to use this library then
> this solution just gets messier. Having 2 different CMake module files for
> the clang and lldb sides each or putting protobuf mutator in a more central
> location is another way to approach this
I agree with Ismail. I think we can fix this issue by making it possible to set
the `PBM_PREFIX` in the scope that includes `ProtobufMutator.cmake` like this:
In `clang/cmake/modules/ProtobufMutator.cmake`:
```
if (NOT PBM_PREFIX)
set (PBM_PREFIX protobuf_mutator)
endif()
```
In `lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt`:
```
set (PBM_PREFIX lldb_protobuf_mutator)
include(ProtobufMutator)
```
That said, I think an even better way to do this is by using a cached variable
that keep track of whether we've already included the external project. That
way we don't have to create two different targets that are essentially the
same. If I'm following the code correctly, all the variables that we rely on
can be computed without the `ExternalProject_Add`. I imagine something like:
```
if (NOT PBM_PROJECT_ADDED)
ExternalProject_Add(${PBM_PREFIX}
PREFIX ${PBM_PREFIX}
GIT_REPOSITORY https://github.com/google/libprotobuf-mutator.git
GIT_TAG master
CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
CMAKE_CACHE_ARGS -DCMAKE_C_COMPILER:FILEPATH=${CMAKE_C_COMPILER}
-DCMAKE_CXX_COMPILER:FILEPATH=${CMAKE_CXX_COMPILER}
BUILD_BYPRODUCTS ${PBM_LIB_PATH} ${PBM_FUZZ_LIB_PATH}
UPDATE_COMMAND ""
INSTALL_COMMAND ""
)
set(PBM_PROJECT_ADDED TRUE CACHE BOOL
"Whether the ProtoBufMutator external project was added")
endif()
```
I think that should preclude us from creating the target twice.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129377/new/
https://reviews.llvm.org/D129377
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
