[bug#71897,v8,6/8] gnu: Add cpp-ada-url-parser.

*
gnu/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch:
New file.
* gnu/local.mk (dist_patch_DATA): Register it.
* gnu/packages/cpp.scm (cpp-ada-url-parser): New variable.

Change-Id: I9924bba53ed58bbf306bf073c9724cd7bd6f570a
---
 gnu/local.mk                                  |   1 +
 gnu/packages/cpp.scm                          |  26 ++++
 ...ser-find-system-testing-dependencies.patch | 130 ++++++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 gnu/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch

Hi Dan,

dan <i@dan.games> writes:

> *
> gnu/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch:
> New file.
> * gnu/local.mk (dist_patch_DATA): Register it.
> * gnu/packages/cpp.scm (cpp-ada-url-parser): New variable.
>
> Change-Id: I9924bba53ed58bbf306bf073c9724cd7bd6f570a
> ---
>  gnu/local.mk                                  |   1 +
>  gnu/packages/cpp.scm                          |  26 ++++
>  ...ser-find-system-testing-dependencies.patch | 130 ++++++++++++++++++
>  3 files changed, 157 insertions(+)
>  create mode 100644 gnu/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch
>
> diff --git a/gnu/local.mk b/gnu/local.mk
> index 802f4be4fe..829c5e166e 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -1103,6 +1103,7 @@ dist_patch_DATA =						\
>    %D%/packages/patches/cool-retro-term-wctype.patch		\
>    %D%/packages/patches/coq-autosubst-1.8-remove-deprecated-files.patch		\
>    %D%/packages/patches/coreutils-gnulib-tests.patch		\
> +  %D%/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch\
>    %D%/packages/patches/cppcheck-disable-char-signedness-test.patch	\
>    %D%/packages/patches/cppdap-add-CPPDAP_USE_EXTERNAL_GTEST_PACKAGE.patch\
>    %D%/packages/patches/cpulimit-with-glib-2.32.patch		\
> diff --git a/gnu/packages/cpp.scm b/gnu/packages/cpp.scm
> index c805dae825..92be49967f 100644
> --- a/gnu/packages/cpp.scm
> +++ b/gnu/packages/cpp.scm
> @@ -3284,3 +3284,29 @@ (define-public tl-optional
>  the std::optional for C++11/14/17, with support for monadic operations added in
>  C++23.")
>      (license license:cc0)))
> +
> +(define-public cpp-ada-url-parser
> +  (package
> +    (name "cpp-ada-url-parser")

I wasn't sure about the naming, since it doesn't match with the upstream
repository name convention, but after looking into it, it seems
reasonable given their own naming is not explicit enough and there are
bindings for various languages.

> +    (version "2.9.2")
> +    (source
> +      (origin
> +        (method git-fetch)
> +        (uri (git-reference
> +              (url "https://github.com/ada-url/ada.git")

I believe 'guix lint' would complain about a URL redirect on the
above. '.git' suffices haven't been used in years for GitHub.

> +              (commit (string-append "v" version))))
> +        (file-name (git-file-name name version))
> +        (sha256 (base32 "0xvvjlia627ajl966gdxzy2b1j0jiimx7zx8ypmffwx0k6x72qam"))
> +        (patches (search-patches "cpp-ada-url-parser-find-system-testing-dependencies.patch"))))

Too long line; please break it so it fits under 80 chars (that's our
convention), although 'guix lint' won't warn until 100 chars.

> +    (build-system cmake-build-system)
> +    (native-inputs
> +     (list cxxopts
> +           fmt
> +           googletest
> +           python
> +           simdjson))
> +    (home-page "https://github.com/ada-url/ada")
> +    (synopsis "URL parser")
> +    (description "Ada is a fast and spec-compliant URL parser written in C++.
> +Specification for URL parser can be found from the WHATWG website.")
> +    (license license:gpl3+)))
> diff --git a/gnu/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch b/gnu/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch
> new file mode 100644
> index 0000000000..b32d162530
> --- /dev/null
> +++ b/gnu/packages/patches/cpp-ada-url-parser-find-system-testing-dependencies.patch
> @@ -0,0 +1,130 @@
> +From 74cac57a3cebe0cfbbc44f46270d5e51986f4881 Mon Sep 17 00:00:00 2001
> +From: dan <i@dan.games>
> +Date: Sat, 21 Sep 2024 18:49:10 +0800
> +Subject: [PATCH] find system testing dependencies.
> +
> +---
> + CMakeLists.txt             | 50 +++++++++-----------------------------
> + singleheader/amalgamate.py |  2 +-
> + tools/cli/CMakeLists.txt   |  8 ++----
> + 3 files changed, 14 insertions(+), 46 deletions(-)
> +
> +diff --git a/CMakeLists.txt b/CMakeLists.txt
> +index a7ce3796..0903cc31 100644
> +--- a/CMakeLists.txt
> ++++ b/CMakeLists.txt
> +@@ -28,43 +28,23 @@ option(ADA_TESTING "Build tests" ${BUILD_TESTING})
> + # errors due to CPM, so this is here to support disabling all the testing
> + # and tooling for ada if one only wishes to use the ada library.
> + if(ADA_TESTING OR ADA_BENCHMARKS OR ADA_TOOLS)
> +-  include(cmake/CPM.cmake)
> +-  # CPM requires git as an implicit dependency
> +-  find_package(Git QUIET)
> +   # We use googletest in the tests
> +-  if(Git_FOUND AND ADA_TESTING)
> +-    CPMAddPackage(
> +-      NAME GTest
> +-      GITHUB_REPOSITORY google/googletest
> +-      VERSION 1.14.0
> +-      OPTIONS  "BUILD_GMOCK OFF" "INSTALL_GTEST OFF"
> +-    )
> ++  if(ADA_TESTING)
> ++    find_package(GTest)
> +   endif()
> +   # We use simdjson in both the benchmarks and tests
> +-  if(Git_FOUND AND (ADA_TESTING OR ADA_BENCHMARKS))
> +-    CPMAddPackage("gh:simdjson/simdjson@3.9.1")
> ++  if(ADA_TESTING OR ADA_BENCHMARKS)
> ++    find_package(simdjson)
> +   endif()
> +   # We use Google Benchmark, but it does not build under several 32-bit systems.
> +-  if(Git_FOUND AND ADA_BENCHMARKS AND (CMAKE_SIZEOF_VOID_P EQUAL 8))
> +-    CPMAddPackage(
> +-      NAME benchmark
> +-      GITHUB_REPOSITORY google/benchmark
> +-      GIT_TAG f91b6b4
> +-      OPTIONS "BENCHMARK_ENABLE_TESTING OFF"
> +-              "BENCHMARK_ENABLE_INSTALL OFF"
> +-              "BENCHMARK_ENABLE_WERROR OFF"
> +-
> +-    )
> ++  if(ADA_BENCHMARKS AND (CMAKE_SIZEOF_VOID_P EQUAL 8))
> ++    find_package(benchmark)
> +   endif()
> + 
> +   if (ADA_TESTING AND NOT EMSCRIPTEN)
> +-    if(Git_FOUND)
> +-      set(CTEST_TEST_TIMEOUT 5)
> +-      message(STATUS "The tests are enabled.")
> +-      add_subdirectory(tests)
> +-    else()
> +-      message(STATUS "The tests are disabled because git was not found.")
> +-    endif()
> ++    set(CTEST_TEST_TIMEOUT 5)
> ++    message(STATUS "The tests are enabled.")
> ++    add_subdirectory(tests)
> +   else()
> +     if(is_top_project)
> +       message(STATUS "The tests are disabled.")
> +@@ -72,12 +52,8 @@ if(ADA_TESTING OR ADA_BENCHMARKS OR ADA_TOOLS)
> +   endif(ADA_TESTING AND NOT EMSCRIPTEN)
> + 
> +   If(ADA_BENCHMARKS AND NOT EMSCRIPTEN)
> +-    if(Git_FOUND)
> +-      message(STATUS "Ada benchmarks enabled.")
> +-      add_subdirectory(benchmarks)
> +-    else()
> +-      message(STATUS "The benchmarks are disabled because git was not found.")
> +-    endif()
> ++    message(STATUS "Ada benchmarks enabled.")
> ++    add_subdirectory(benchmarks)
> +   else(ADA_BENCHMARKS AND NOT EMSCRIPTEN)
> +     if(is_top_project)
> +       message(STATUS "Ada benchmarks disabled. Set ADA_BENCHMARKS=ON to enable them.")
> +@@ -107,11 +83,7 @@ if(NOT ADA_COVERAGE AND NOT EMSCRIPTEN)
> + endif()
> + 
> + if(ADA_TOOLS)
> +-  if(Git_FOUND)
> +     add_subdirectory(tools)
> +-  else()
> +-    message(STATUS "The tools are disabled because git was not found.")
> +-  endif()
> + endif()
> + 
> + install(
> +diff --git a/singleheader/amalgamate.py b/singleheader/amalgamate.py
> +index 52b154b6..94e4e638 100755
> +--- a/singleheader/amalgamate.py
> ++++ b/singleheader/amalgamate.py
> +@@ -138,7 +138,7 @@ if SCRIPTPATH != AMALGAMATE_OUTPUT_PATH:
> + 
> + shutil.copy2(os.path.join(AMALGAMATE_INCLUDE_PATH, 'ada_c.h'), AMALGAMATE_OUTPUT_PATH)
> + 
> +-zf = zipfile.ZipFile(os.path.join(AMALGAMATE_OUTPUT_PATH, 'singleheader.zip'), 'w', zipfile.ZIP_DEFLATED)
> ++zf = zipfile.ZipFile(os.path.join(AMALGAMATE_OUTPUT_PATH, 'singleheader.zip'), 'w', zipfile.ZIP_DEFLATED, strict_timestamps=False)
> + zf.write(os.path.join(AMALGAMATE_OUTPUT_PATH, 'ada.cpp'), 'ada.cpp')
> + zf.write(os.path.join(AMALGAMATE_OUTPUT_PATH, 'ada.h'), 'ada.h')
> + zf.write(os.path.join(AMALGAMATE_INCLUDE_PATH, 'ada_c.h'), 'ada_c.h')
> +diff --git a/tools/cli/CMakeLists.txt b/tools/cli/CMakeLists.txt
> +index 9f0da167..d0f7e0c9 100644
> +--- a/tools/cli/CMakeLists.txt
> ++++ b/tools/cli/CMakeLists.txt
> +@@ -8,12 +8,8 @@ if(MSVC AND BUILD_SHARED_LIBS)
> +         "$<TARGET_FILE:ada>"      # <--this is in-file
> +         "$<TARGET_FILE_DIR:adaparse>")                 # <--this is out-file path
> + endif()
> +-CPMAddPackage("gh:fmtlib/fmt#10.2.1")
> +-CPMAddPackage(
> +-  GITHUB_REPOSITORY jarro2783/cxxopts
> +-  VERSION 3.2.0
> +-  OPTIONS "CXXOPTS_BUILD_EXAMPLES NO" "CXXOPTS_BUILD_TESTS NO" "CXXOPTS_ENABLE_INSTALL YES"
> +-)
> ++find_package(fmt)
> ++find_package(cxxopts)
> + target_link_libraries(adaparse PRIVATE cxxopts::cxxopts fmt::fmt)
> + 
> + if(MSVC OR MINGW)
> +-- 
> +2.45.2
> +

That's not upstreamable work, so should ideally be marked as such as a
patch at the top of the patch file (Upstream-status: N/A or similar).
It's a bit sad that this will need ongoing maintenance (careful rebasing
when they touch that file).  I'd like to see the problem with CPM
commented in a bit more details -- wondering if it could be made to work
without patching it out, e.g. perhaps some CMake variables can be set to
have CPM work offline?

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> That's not upstreamable work, so should ideally be marked as 
> such as a
> patch at the top of the patch file (Upstream-status: N/A or 
> similar).
> It's a bit sad that this will need ongoing maintenance (careful 
> rebasing
> when they touch that file).  I'd like to see the problem with 
> CPM
> commented in a bit more details -- wondering if it could be made 
> to work
> without patching it out, e.g. perhaps some CMake variables can 
> be set to
> have CPM work offline?

In the previous iteration, I disabled ADA_TESTING and ADA_TOOLS so 
that no patches are needed.  However, Liliana suggest that "we 
should enable testing, even if we need to patch the cmake files to 
unvendor inputs".  I think we have to make a decision here: we 
either accept a package with testing disabled, or we need extra 
work to maintain the patch in the future.

I took a brief look at CPM, and it seems possible to fetch 
dependencies from local directories, but I assume even we go this 
way we still need to patch their CMakeLists.txt files.

What do you think?

Hi Dan,

dan <i@dan.games> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> That's not upstreamable work, so should ideally be marked as such as
>> a
>> patch at the top of the patch file (Upstream-status: N/A or
>> similar).
>> It's a bit sad that this will need ongoing maintenance (careful
>> rebasing
>> when they touch that file).  I'd like to see the problem with CPM
>> commented in a bit more details -- wondering if it could be made to
>> work
>> without patching it out, e.g. perhaps some CMake variables can be
>> set to
>> have CPM work offline?
>
> In the previous iteration, I disabled ADA_TESTING and ADA_TOOLS so
> that no patches are needed.  However, Liliana suggest that "we should
> enable testing, even if we need to patch the cmake files to unvendor
> inputs".  I think we have to make a decision here: we either accept a
> package with testing disabled, or we need extra work to maintain the
> patch in the future.

I agree with Liliana.  We strive to enable test suites in Guix packages,
as this gives us a much easier time detecting breakages early when
upgrading dependent packages.

My suggestion was to look a little bit deeper in the problem at hand in
case to see if it can be configured to work for our needs.  If it can't,
then your approach is perfectly fine (ideally with a feature request
upstream requesting an easier way to run their test suite offline); if
it can, we should configure it instead and avoid having to maintain
extra patches.

> I took a brief look at CPM, and it seems possible to fetch
> dependencies from local directories, but I assume even we go this way
> we still need to patch their CMakeLists.txt files.

There's a relatively new FetchContent module in CMake that perhaps CPM
abstracts?  If it's using FetchContent, it should be possible to have
the build system attempt to use a system-provided library instead of
fetching from a submodule and building it from source.  See the 'jami'
package, for example, which sets
'-DFETCHCONTENT_TRY_FIND_PACKAGE_MODE=ALWAYS' to have FetchContent do
that.