cmake: support disabling CPM #557
Conversation
|
I highly recommend reviewing with whitespace diff off: https://github.com/ada-url/ada/pull/557/files?w=1 |
|
The test failure seem to be the same as on |
|
@rockwotj We have a flaky CI on Windows. It's unrelated 👍 |
|
Your description mentions demo.cpp, demo.c files. You shouldn't even compile that as an embedder. Would you mind taking a look at it as well? |
Thanks I missed an Are you suggesting you shouldn't generate the single header version in an embedded situation? I think we'd need to add a flag to disable that generation then, because it seems that always happens if Python3 is available. |
When using ada as a library and embedding it in our project via
FetchContent, we've run into errors like this:
```
[253/1413] Generating ada.cpp, ada.h, ada_c.h, demo.cpp, demo.c, README.md
fatal: detected dubious ownership in repository at '/src/redpanda'
```
In our CI environment we've seen flaky HTTP issues as well:
```
CMake Error at build/_deps/ada-src/cmake/CPM.cmake:19 (file):
file DOWNLOAD cannot compute hash on failed download
status: [22;"HTTP response code said error"]
Call Stack (most recent call first):
build/_deps/ada-src/CMakeLists.txt:27 (include)
CMake Error at build/_deps/ada-src/CMakeLists.txt:32 (CPMAddPackage):
Unknown CMake command "CPMAddPackage".
```
Actually building just the library seems very simple and uses vanilla
cmake. CPM is only used for testing/benchmarking/etc. We've seen some of
the dubious ownership issues just including CPM (hacking around trying
to disable GIT failed to fix the issue).
To support our usecase, I've added a flag allowing disabling tests, the
default value is the value of BUILD_TESTING so hopefully nothing changes
for anyone.
I tested this via the following commands:
```
cmake -B build && cmake --build build
cmake -B build -DADA_BENCHMARKS=ON && cmake --build build
cmake -B build -DADA_TESTING=OFF -DADA_TOOLS=OFF -DADA_BENCHMARKS=OFF && cmake --build build
```
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
|
Force push: Address review feedback |
|
@anonrig If this passes our tests, then it seems rather uncontroversial. We went from having our own approach to downloading dependencies, to using CPMAddPackage. We should run additional tests. |
|
Can you verify that the issue is on our end, and not with your build? I seem to be able to use fetchcontent fine: https://github.com/ada-url/ada_fetchcontent Can you produce a simple example where it fails? |
My understanding is that the build doesn't fail. Apologies, I did not make that clear. The main issue from my understanding is CPM's usage of git. We mount and build our code within containers and when adding ada we get messages like: Which based on git/git@8959555 means that CPM shelling out to Additionally, we have seen some CI failures due to the downloading of dependencies that CPM does, here is an example message (we vendor all of our transitive dependencies in the environment, and it's a little annoying that we have to vendor test dependencies for ADA when we just want to use the library). That being said, I think it's valid if you don't want to accept this patch, we can figure out how to either vendor the single header version or something similar. |
|
The patch is fine. I do not object. I am puzzled by the CI failure, but I don’t think it is related to this PR. |
When using ada as a library and embedding it in our project via
FetchContent, we've run into errors like this:
In our CI environment we've seen flaky HTTP issues as well:
Actually building just the library seems very simple and uses vanilla
cmake. CPM is only used for testing/benchmarking/etc. We've seen some of
the dubious ownership issues just including CPM (hacking around trying
to disable GIT failed to fix the issue).
To support our usecase, I've added a flag allowing disabling tests, the
default value is the value of BUILD_TESTING so hopefully nothing changes
for anyone.
I tested this via the following commands: