Add curl.cmake dependency for cpr #7853

liujiayi771 · 2023-12-04T11:50:37Z

Add a curl.cmake dependency in CMake/resolve_dependency_modules, so that
we would be able to specify VELOX_CURL_URL to a local path to enable offline
build.

netlify · 2023-12-04T11:50:43Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`79cedd0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/656e0eb452b91b00096246f5

liujiayi771 · 2023-12-04T11:51:25Z

@duanmeng Can we add a curl.cmake dependency for cpr offline build?

duanmeng · 2023-12-04T12:48:11Z

CMake/resolve_dependency_modules/cpr.cmake

@@ -20,6 +20,9 @@ set(VELOX_CPR_SOURCE_URL
    "https://github.com/libcpr/cpr/archive/refs/tags/${VELOX_CPR_VERSION}.tar.gz"
 )

+set(curl_SOURCE BUNDLED)
+resolve_dependency(curl)


Could this disable the curl installation in cpr?
https://github.com/libcpr/cpr/blob/master/CMakeLists.txt#L174

Yes, you can refer to the sentence "the first such call will control how that dependency will be made available" in this link.

I think it's enough to use FetchContent_Declare to define VELOX_CURL_SOURCE_URL without calling FetchContent_MakeAvailable, because cpr's CMakeLists.txt defines many CURL compilation parameters. Our purpose is just to define the CURL source URL, and the compilation triggering is still done by cpr calling FetchContent_MakeAvailable.

I can still see the curl building log in cpr building, could help double check?
https://app.circleci.com/pipelines/github/facebookincubator/velox/39899/workflows/20a0d56a-51b9-4832-8d9f-96e5685a33f2/jobs/271655/parallel-runs/0/steps/0-106?invite=true#step-106-40221_31

This is just a log in the cmake of cpr, and it will always be printed out. I just removed FetchContent_MakeAvailable in curl.cmake, so the compilation process is triggered by cpr with the correct compilation parameters. The role of curl.cmake in Velox is simply to define the source URL of curl through FetchContent_Declare.

Still using the built-in curl, but now we can specify the source URL and version of curl.

duanmeng · 2023-12-04T12:50:58Z

@duanmeng Can we add a curl.cmake dependency for cpr offline build?

@liujiayi771 LGTM.

@assignUser Hi Jacob, what do you think, could you please help take a look?

assignUser · 2023-12-04T13:23:12Z

From the other pr:

Adding a custom curl version is likely to lead to conflicts with host openssl/curl but you can give it a try ^^

Alternatively you could manually pass -DCPR_USE_SYSTEM_CURL=ON to the build if you don't want to use it.

assignUser · 2023-12-04T13:26:11Z

I'll have to review in-depth later but like this it seems like nothing is using the build curl version.

liujiayi771 · 2023-12-04T14:50:50Z

@assignUser The operating system may have installed a version of curl that is incompatible with Velox. The purpose of this modification is to support configuring the source URL for curl. I still want to use the corresponding version of curl with cpr, and this modification can enable support for specifying a URL for curl to a local file, allowing for the compilation of curl in offline environments. Otherwise, in an environment without internet access, it would not be possible to download the specified curl source link in cpr CMakeLists.txt.

liujiayi771 · 2023-12-04T16:27:53Z

According to #4325, we can use export VELOX_CURL_URL=/path/to/local/curl-8.4.0.tar.xz to specify the path of the local curl source package, eliminating the need to download the source package from the internet.

assignUser

I misunderstood your intention earlier but it is clear now. Great job using fc_declares 'cached' nature, very clever!

Looking at the ci logs cpr seems to use the version set here (8.4.0). @liujiayi771 have you confirmed that using a tar.gz works as intended as well?

A minor comment, lgtm.

assignUser · 2023-12-04T16:46:56Z

CMake/resolve_dependency_modules/cpr.cmake

@@ -20,6 +20,9 @@ set(VELOX_CPR_SOURCE_URL
    "https://github.com/libcpr/cpr/archive/refs/tags/${VELOX_CPR_VERSION}.tar.gz"
 )

+set(curl_SOURCE BUNDLED)


👍 placing that in the cpr file! could you add a comment explaining what this does?

duanmeng · 2023-12-04T17:02:30Z

@kgpai Hi Krishna, could you help review and merge this PR? Thanks.

liujiayi771 · 2023-12-04T17:30:11Z

have you confirmed that using a tar.gz works as intended as well?

@assignUser Yes. If I set VELOX_CURL_URL=/path/to/curl-8.3.0.tar.gz, I can see in the cpr building logs that the version of curl has changed to 8.3.0.

assignUser · 2023-12-04T17:35:18Z

CMake/resolve_dependency_modules/cpr.cmake

@@ -20,6 +20,11 @@ set(VELOX_CPR_SOURCE_URL
    "https://github.com/libcpr/cpr/archive/refs/tags/${VELOX_CPR_VERSION}.tar.gz"
 )

+# Add the dependency for curl, so that we can define the source URL
+# for curl in curl.cmake.


Suggested change

# for curl in curl.cmake.

# for curl in curl.cmake. This will override the curl version declared by cpr.

just a little tweak

liujiayi771 · 2023-12-07T09:33:56Z

@mbasmanova @kgpai Could you help review?

mbasmanova · 2023-12-07T12:17:59Z

@kgpai Krishna, would you help review this PR?

kgpai

Looks good.

One question for @assignUser this shouldnt happen with condafication where we would be easily be able to set curl version along with cpr version, without having to build them together like this , right ?

facebook-github-bot · 2023-12-08T00:20:08Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-12-08T01:12:00Z

@kgpai merged this pull request in b908e77.

assignUser · 2023-12-08T01:15:17Z

@kgpai Yes, cpr can either use a system version of curl of download a custom one (which this PR let's us influence without having to patch cpr), with conda we would make it use the system version that we installed via conda.

conbench-facebook · 2023-12-08T01:32:28Z

Conbench analyzed the 1 benchmark run on commit b908e779.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: After #7853, compiling cpr still takes up a significant amount of time because cpr try fetch and compile ZLIB, which breaks offline build. However, ZLIB has already been found by find_package(ZLIB, REQUIRED), let's avoid recompile ZLIB when compiling curl for cpr. https://github.com/libcpr/cpr/blob/51915127d9b4b9f75965b239a5cd7a765722d1df/CMakeLists.txt#L202-L209 Pull Request resolved: #8257 Reviewed By: xiaoxmeng Differential Revision: D52631339 Pulled By: kgpai fbshipit-source-id: 57a435a6f550f9fa08e685d2805b5aceea61e480

Summary: After facebookincubator#7853, compiling cpr still takes up a significant amount of time because cpr try fetch and compile ZLIB, which breaks offline build. However, ZLIB has already been found by find_package(ZLIB, REQUIRED), let's avoid recompile ZLIB when compiling curl for cpr. https://github.com/libcpr/cpr/blob/51915127d9b4b9f75965b239a5cd7a765722d1df/CMakeLists.txt#L202-L209 Pull Request resolved: facebookincubator#8257 Reviewed By: xiaoxmeng Differential Revision: D52631339 Pulled By: kgpai fbshipit-source-id: 57a435a6f550f9fa08e685d2805b5aceea61e480

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 4, 2023

liujiayi771 mentioned this pull request Dec 4, 2023

Do not use system curl to build cpr #7718

Closed

liujiayi771 force-pushed the curl-cmake branch from 48a5a4c to c9456da Compare December 4, 2023 12:12

duanmeng reviewed Dec 4, 2023

View reviewed changes

duanmeng requested a review from assignUser December 4, 2023 12:51

Add curl.cmake dependency for cpr

3578b3d

liujiayi771 force-pushed the curl-cmake branch from c9456da to 3578b3d Compare December 4, 2023 13:59

liujiayi771 added 2 commits December 4, 2023 23:07

remove FetchContent_MakeAvailable for curl

b12c814

remove message

0bb799f

assignUser approved these changes Dec 4, 2023

View reviewed changes

duanmeng requested a review from kgpai December 4, 2023 17:02

liujiayi771 force-pushed the curl-cmake branch from 8d10d6a to 66eb6a1 Compare December 4, 2023 17:32

assignUser reviewed Dec 4, 2023

View reviewed changes

liujiayi771 force-pushed the curl-cmake branch from 66eb6a1 to 1d5737a Compare December 4, 2023 17:37

add comments in cpr.cmake

79cedd0

liujiayi771 force-pushed the curl-cmake branch from 1d5737a to 79cedd0 Compare December 4, 2023 17:38

duanmeng mentioned this pull request Dec 5, 2023

curl dependency build is showing many deprecated-declarations warnings on MacOS #7881

Closed

kgpai approved these changes Dec 8, 2023

View reviewed changes

facebook-github-bot closed this in b908e77 Dec 8, 2023

facebook-github-bot added the Merged label Dec 8, 2023

zhouyuan mentioned this pull request Jan 4, 2024

[VL] Disable system curl to build cpr apache/incubator-gluten#4262

Merged

Yohahaha mentioned this pull request Jan 4, 2024

Disable build ZLIB when building cpr #8257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add curl.cmake dependency for cpr #7853

Add curl.cmake dependency for cpr #7853

liujiayi771 commented Dec 4, 2023 •

edited

netlify bot commented Dec 4, 2023 •

edited

liujiayi771 commented Dec 4, 2023 •

edited

duanmeng Dec 4, 2023

liujiayi771 Dec 4, 2023

liujiayi771 Dec 4, 2023 •

edited

duanmeng Dec 4, 2023

liujiayi771 Dec 4, 2023 •

edited

liujiayi771 Dec 4, 2023

duanmeng commented Dec 4, 2023

assignUser commented Dec 4, 2023

assignUser commented Dec 4, 2023

liujiayi771 commented Dec 4, 2023 •

edited

liujiayi771 commented Dec 4, 2023 •

edited

assignUser left a comment

assignUser Dec 4, 2023

liujiayi771 Dec 4, 2023

duanmeng commented Dec 4, 2023

liujiayi771 commented Dec 4, 2023

assignUser Dec 4, 2023

liujiayi771 commented Dec 7, 2023

mbasmanova commented Dec 7, 2023

kgpai left a comment

facebook-github-bot commented Dec 8, 2023

facebook-github-bot commented Dec 8, 2023

assignUser commented Dec 8, 2023

conbench-facebook bot commented Dec 8, 2023

	# for curl in curl.cmake.
	# for curl in curl.cmake. This will override the curl version declared by cpr.

Add curl.cmake dependency for cpr #7853

Add curl.cmake dependency for cpr #7853

Conversation

liujiayi771 commented Dec 4, 2023 • edited

netlify bot commented Dec 4, 2023 • edited

✅ Deploy Preview for meta-velox canceled.

liujiayi771 commented Dec 4, 2023 • edited

duanmeng Dec 4, 2023

Choose a reason for hiding this comment

liujiayi771 Dec 4, 2023

Choose a reason for hiding this comment

liujiayi771 Dec 4, 2023 • edited

Choose a reason for hiding this comment

duanmeng Dec 4, 2023

Choose a reason for hiding this comment

liujiayi771 Dec 4, 2023 • edited

Choose a reason for hiding this comment

liujiayi771 Dec 4, 2023

Choose a reason for hiding this comment

duanmeng commented Dec 4, 2023

assignUser commented Dec 4, 2023

assignUser commented Dec 4, 2023

liujiayi771 commented Dec 4, 2023 • edited

liujiayi771 commented Dec 4, 2023 • edited

assignUser left a comment

Choose a reason for hiding this comment

assignUser Dec 4, 2023

Choose a reason for hiding this comment

liujiayi771 Dec 4, 2023

Choose a reason for hiding this comment

duanmeng commented Dec 4, 2023

liujiayi771 commented Dec 4, 2023

assignUser Dec 4, 2023

Choose a reason for hiding this comment

liujiayi771 commented Dec 7, 2023

mbasmanova commented Dec 7, 2023

kgpai left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 8, 2023

facebook-github-bot commented Dec 8, 2023

assignUser commented Dec 8, 2023

conbench-facebook bot commented Dec 8, 2023

liujiayi771 commented Dec 4, 2023 •

edited

netlify bot commented Dec 4, 2023 •

edited

liujiayi771 commented Dec 4, 2023 •

edited

liujiayi771 Dec 4, 2023 •

edited

liujiayi771 Dec 4, 2023 •

edited

liujiayi771 commented Dec 4, 2023 •

edited

liujiayi771 commented Dec 4, 2023 •

edited