3rdparty: Disable tests and tools for ada #14618

BenPope · 2023-11-02T11:08:58Z

ada uses CPM: https://github.com/cpm-cmake/CPM.cmake

Disable ADA_TESTING, ADA_TOOLS, ADA_BENCHMARKING to prevent CPM from downloading 3rdparty repositories.

I'm still seeing this message:

Generating ada.cpp, ada.h, ada_c.h, demo.cpp, demo.c, README.md
fatal: detected dubious ownership in repository at '/var/lib/buildkite-agent/builds/buildkite-amd64-builders-i-0d17fc136ea6bdd74-1/redpanda/redpanda'

Also add the license.

Backports Required

Release Notes

none

vbotbuildovich · 2023-11-02T14:00:28Z

new failures detected in https://buildkite.com/redpanda/redpanda/builds/40302#018b9014-082f-457f-8d12-f89d7d17bf4e: "rptest.tests.rpk_registry_test.RpkRegistryTest.test_produce_consume_proto"

rockwotj

Maybe they would take patches to allow disabling testing via an ADA_TESTING option, I don't love the workarounds here, but alas cmake.

Should we be doing this for vtools too?

rockwotj · 2023-11-02T14:08:02Z

cmake/dependencies.cmake

+  FetchContent_Populate(ada)
+  set(GIT_EXECUTABLE_OLD ${GIT_EXECUTABLE})
+  set(GIT_EXECUTABLE "" CACHE INTERNAL "Invalid path to the Git executable")
+  add_subdirectory(${ada_SOURCE_DIR} ${ada_BINARY_DIR})
+  set(GIT_EXECUTABLE ${GIT_EXECUTABLE_OLD} CACHE STRING "location of git" FORCE)


This seems not fun.

Should we be setting BUILD_TESTING to OFF instead of this hack around disabling the git exec lookup?

Additionally we should be able to set(ADA_TOOLS OFF) to disable building the tools right? That feels like a slightly better solution, but ideally they had an option like ADA_TESTING instead of BUILD_TESTING right?

I guess this ends up being net less because we'll have to do the same BUILD_TESTING hack to disable and re-enable as we only want tests in our repo.

I think you mean like before I force pushed

I was seeing some git activity, and was trying to get rid of: "detected dubious ownership in repository at"

I'm ambivalent. Ideally we just did something like:

set(ADA_TOOLS OFF) and set(ADA_TESTING OFF) and called it a day (without the set it back hack). I'm fine with this, but would love Noah's opinion here. I'm a CMake newbie.

I was seeing some git activity, and was trying to get rid of: "detected dubious ownership in repository at"

Did this fix it?

There's no git activity now! But same error.

Bummer...

It looks like they distribute a single header version in releases... Is it easy in CMake to download that and add it as a target?

https://github.com/ada-url/ada/releases/tag/v2.7.2

Anyways I'm not trying to add more work for you, we can just start with this.

vbotbuildovich · 2023-11-02T14:15:51Z

new failures detected in https://buildkite.com/redpanda/redpanda/builds/40302#018b9023-0464-4a30-b3aa-8ad2feb8f29e: "rptest.tests.topic_recovery_test.TopicRecoveryTest.test_time_based_retention.cloud_storage_type=CloudStorageType.ABS"

BenPope · 2023-11-02T14:21:37Z

Should we be doing this for vtools too?

Yeah, I'll make equivalent changes when this is merged.

dotnwat · 2023-11-02T14:51:09Z

cmake/dependencies.cmake

+FetchContent_GetProperties(ada)
+if(NOT ada_POPULATED)
+  # Disable git to prevent CPM from downloading 3rdparty repositories.
+  # An intentional side effect is that tests and tools are not built
+
+  FetchContent_Populate(ada)
+  set(GIT_EXECUTABLE_OLD ${GIT_EXECUTABLE})
+  set(GIT_EXECUTABLE "" CACHE INTERNAL "Invalid path to the Git executable")
+  add_subdirectory(${ada_SOURCE_DIR} ${ada_BINARY_DIR})
+  set(GIT_EXECUTABLE ${GIT_EXECUTABLE_OLD} CACHE STRING "location of git" FORCE)


the simplest thing to do is fork ada and write a better cmake file. i did it for several of the dependencies in here, and it is pretty easy since we don't have maintain tests, benchmarks, installation, etc...

alternatively, you can check-in a patch to the cmakefile and have fetchcontent apply the patch.

I think @travisdowns knows one of the maintainers, so maybe we can get them to apply the patch (or just write a dumb CMake file for the single file version).

yeh if we want to go through upstream that works. the problems with the ada cmake file are pretty reasonable i would expect maintainers to pick fixes for them. here is the one i did for Avro https://github.com/redpanda-data/avro/blob/release-1.11.1-redpanda/redpanda_build/CMakeLists.txt as an example of a custom cmakefile.

@dotnwat Any objection to merging this as-is to mitigate this type of failure and then reworking the build in followup work?

vbotbuildovich · 2023-11-02T15:31:11Z

new failures detected in https://buildkite.com/redpanda/redpanda/builds/40302#018b906e-19e6-41a5-b6c0-54cb82495fb3: "rptest.tests.partition_movement_test.PartitionMovementTest.test_deletion_stops_move.num_to_upgrade=2"

BenPope · 2023-11-02T17:13:01Z

CI Failures are unrelated as this just affects the public build.

anonrig · 2023-11-08T06:20:30Z

Maybe they would take patches to allow disabling testing via an ADA_TESTING option, I don't love the workarounds here, but alas cmake.

We are open to any patches :-)

rockwotj · 2023-11-08T16:07:49Z

@anonrig Wow I don't know if someone pointed you here, but 🙏

I created a PR to what I believe would work for us: ada-url/ada#557

@BenPope and @dotnwat feel free to take a peek at that patch, I believe that should work here, but you're both better at CMake than I am.

ada uses CPM: https://github.com/cpm-cmake/CPM.cmake Disable ADA_TESTING, ADA_TOOLS, ADA_BENCHMARKING to prevent CPM from downloading 3rdparty repositories. Also add the license. Signed-off-by: Ben Pope <ben@redpanda.com>

rockwotj

Are you still seeing the issue on dubious ownership? If not, please update the cover letter.

BenPope · 2023-11-10T12:30:45Z

Are you still seeing the issue on dubious ownership? If not, please update the cover letter.

Yes, because the single header is always built.

vbotbuildovich · 2023-11-10T13:44:11Z

new failures in https://buildkite.com/redpanda/redpanda/builds/40829#018bb942-82e6-4915-ae94-35321b5ee95c:

"rptest.tests.cloud_storage_scrubber_test.CloudStorageScrubberTest.test_scrubber.cloud_storage_type=CloudStorageType.ABS"

BenPope requested review from dotnwat and rockwotj November 2, 2023 11:08

BenPope requested review from a team, emaxerrno and dswang as code owners November 2, 2023 11:08

BenPope requested review from nk-87 and removed request for a team November 2, 2023 11:08

BenPope self-assigned this Nov 2, 2023

github-actions bot added the area/build label Nov 2, 2023

BenPope force-pushed the 3rdparty_ada_url_no_cpm branch from f533a85 to ee4caca Compare November 2, 2023 12:00

rockwotj previously approved these changes Nov 2, 2023

View reviewed changes

dotnwat reviewed Nov 2, 2023

View reviewed changes

nk-87 previously approved these changes Nov 8, 2023

View reviewed changes

3rdparty: Disable tests and tools for ada

ad41585

ada uses CPM: https://github.com/cpm-cmake/CPM.cmake Disable ADA_TESTING, ADA_TOOLS, ADA_BENCHMARKING to prevent CPM from downloading 3rdparty repositories. Also add the license. Signed-off-by: Ben Pope <ben@redpanda.com>

BenPope dismissed stale reviews from nk-87 and rockwotj via ad41585 November 10, 2023 11:15

BenPope force-pushed the 3rdparty_ada_url_no_cpm branch from ee4caca to ad41585 Compare November 10, 2023 11:15

BenPope requested review from dotnwat and rockwotj November 10, 2023 11:16

anonrig approved these changes Nov 10, 2023

View reviewed changes

rockwotj approved these changes Nov 10, 2023

View reviewed changes

BenPope merged commit 912d051 into redpanda-data:dev Nov 13, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3rdparty: Disable tests and tools for ada #14618

3rdparty: Disable tests and tools for ada #14618

BenPope commented Nov 2, 2023 •

edited

Loading

vbotbuildovich commented Nov 2, 2023

rockwotj left a comment

rockwotj Nov 2, 2023

BenPope Nov 2, 2023

BenPope Nov 2, 2023

rockwotj Nov 2, 2023

rockwotj Nov 2, 2023

BenPope Nov 2, 2023

rockwotj Nov 2, 2023

vbotbuildovich commented Nov 2, 2023

BenPope commented Nov 2, 2023

dotnwat Nov 2, 2023

dotnwat Nov 2, 2023

rockwotj Nov 2, 2023

dotnwat Nov 2, 2023

BenPope Nov 2, 2023

vbotbuildovich commented Nov 2, 2023

BenPope commented Nov 2, 2023 •

edited

Loading

anonrig commented Nov 8, 2023

rockwotj commented Nov 8, 2023

rockwotj left a comment

BenPope commented Nov 10, 2023

vbotbuildovich commented Nov 10, 2023

3rdparty: Disable tests and tools for ada #14618

3rdparty: Disable tests and tools for ada #14618

Conversation

BenPope commented Nov 2, 2023 • edited Loading

Backports Required

Release Notes

vbotbuildovich commented Nov 2, 2023

rockwotj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Nov 2, 2023

BenPope commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Nov 2, 2023

BenPope commented Nov 2, 2023 • edited Loading

anonrig commented Nov 8, 2023

rockwotj commented Nov 8, 2023

rockwotj left a comment

Choose a reason for hiding this comment

BenPope commented Nov 10, 2023

vbotbuildovich commented Nov 10, 2023

BenPope commented Nov 2, 2023 •

edited

Loading

BenPope commented Nov 2, 2023 •

edited

Loading