Skip to content

Use CMake FetchContent for curl #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Use CMake FetchContent for curl #73

merged 3 commits into from
Dec 4, 2023

Conversation

dmehala
Copy link
Collaborator

@dmehala dmehala commented Dec 1, 2023

Description

Rework curl dependency. Now, we use FetchContent which populates dependencies during the configuration stage and bringing all the targets declared by curl.

How does it fix DataDog/nginx-datadog#53?

The build system is now able to bring transitive dependencies into all targets that declare curl as a dependency.

Note to reviewer

I also added the equivalent of --without-zstd.

Add and expose curl as a cmake target
@dmehala dmehala requested review from cgilmour and dgoffredo December 1, 2023 10:27
@pr-commenter
Copy link

pr-commenter bot commented Dec 1, 2023

Benchmarks

Benchmark execution time: 2023-12-03 17:40:42

Comparing candidate commit e9df5e1 in PR branch dmehala/curl-cmake with baseline commit 5c53143 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

Copy link
Contributor

@dgoffredo dgoffredo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

The resulting .so, on my system, is larger now.

Before:

-rwxrwxr-x 1 david david 21943168 Dec  1 11:20 .build/libdd_trace_cpp.so

After:

-rwxrwxr-x 1 david david 23314640 Dec  1 11:18 .build/libdd_trace_cpp.so

I wonder if this is due to the CMake-flavored libcurl build including more code than the configure-based one.

As we've discussed before, the only concern I have with these changes is that they allow dependencies to creep into the build, unnecessary and unnoticed.

I think it's fine, though. This is good to merge.

@dmehala dmehala merged commit 12b1c7c into main Dec 4, 2023
@dmehala dmehala deleted the dmehala/curl-cmake branch December 4, 2023 09:14
dmehala added a commit that referenced this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined symbol: ZSTD_isError on AL2023
2 participants