Skip to content
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

curl package upgraded to 7.81.0 #35130

Merged
merged 5 commits into from
Apr 8, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 8, 2022

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
To fix vulnerabilities reported by WhiteSource

Information about CI checks: https://clickhouse.tech/docs/en/development/continuous-integration/

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 8, 2022
@alexey-milovidov alexey-milovidov self-assigned this Mar 9, 2022
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Mar 9, 2022
@alexey-milovidov
Copy link
Member

LGTM.

But the proper solution will be to remove curl.
It is used only to send data to sentry. We should replace it to something minimal or remove altogether.

Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

@Meena-Renganathan It does not build on Mac OS X,
see the "special build check".

@alexey-milovidov alexey-milovidov marked this pull request as draft March 9, 2022 09:15
@alexey-milovidov
Copy link
Member

@Meena-Renganathan Let's fix the build or close this.

@ghost
Copy link
Author

ghost commented Mar 11, 2022

Sorry, I'm working on it. I will make the changes to fix the build error soon.

@ghost
Copy link
Author

ghost commented Mar 16, 2022

Hi @alexey-milovidov, I've tried to reproduce the error by building curl (with openssl option) in my local MacOS but it's getting built successfully. Here is the details of my MacOS:

macOS Monterey
Clang Version 13.

CC libcurl_la-hostip.lo
CC libcurl_la-hostip4.lo
CC libcurl_la-hostip6.lo
CC libcurl_la-hostsyn.lo

Could you help how it can take it up further to get this fixed?

@alexey-milovidov
Copy link
Member

Let's check the build log: https://s3.amazonaws.com/clickhouse-builds/35130/d19090fdaf89162ba5527f7f376394a6cdd4081f/binary_darwin/build_log.log

Maybe it is related to some defines.
We can investigate it by running exactly the same compiler command.

Also another suggestion: maybe remove curl instead of updating it?

@ghost
Copy link
Author

ghost commented Mar 16, 2022

Thank you @alexey-milovidov. But, while trying upgrade the curl package to 7.81.0, observed the curl is getting referenced by sentry-native and azure modules. So, decided to upgrade the curl.

1 similar comment
@ghost
Copy link
Author

ghost commented Mar 16, 2022

Thank you @alexey-milovidov. But, while trying upgrade the curl package to 7.81.0, observed the curl is getting referenced by sentry-native and azure modules. So, decided to upgrade the curl.

@alexey-milovidov
Copy link
Member

Is it possible to remove curl from Azure?

@ghost
Copy link
Author

ghost commented Mar 16, 2022

It is not possible to directly remove the curl dependency from azure.

Azure module uses the libcurl and WinHTTP libraries as HTTP stacks for communicating with Azure services over the network. When no specific configuration is set, the default transport adapter will be selected based on the OS. But custom transport adapter needs to be implemented to avoid the dependency on curl.

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Mar 18, 2022

Ok, then we need to fix this remaining build.

PS. There was similar problem in AWS library. They used curl and it led to buffering all data in memory (I believe that it is not a downside of a curl, it is actually a good, very well tested library), and we replaced it by Poco. Maybe do similar with Azure?

@alexey-milovidov
Copy link
Member

Still not fixed.

@ghost
Copy link
Author

ghost commented Mar 25, 2022

Hi @alexey-milovidov, yeah noticed the error and working on it to fix.

@alexey-milovidov alexey-milovidov marked this pull request as ready for review March 27, 2022 00:24
set(CURL_LIBS "")
if(APPLE)

find_library(SYSTEMCONFIGURATION_FRAMEWORK "SystemConfiguration")
Copy link
Member

Choose a reason for hiding this comment

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

We don't allow find_library in our cmake files.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @alexey-milovidov, any specific reason we are restricting the find_library in cmake files?

Copy link
Member

Choose a reason for hiding this comment

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

It is needed for hermetic builds - to avoid dependence on the environment.

Copy link
Member

Choose a reason for hiding this comment

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

If the new curl version started to need something from SystemConfiguration,
maybe better to chop it from the library?

@alexey-milovidov
Copy link
Member

I did not check, but need to figure out where exactly these dependencies are needed? Is it possible to avoid them?

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Apr 1, 2022

@Meena-Renganathan
Undefining this macro would help:
https://github.com/curl/curl/blob/8a1fa3b364b9db52cf3e8d843ac1e749890985e6/lib/hostip.c#L71

@ghost
Copy link
Author

ghost commented Apr 1, 2022

Hi @alexey-milovidov,

To get better clarity, underlining the macro ENABLE_IPV6 doesn't allow to support IPV6 addressing format in ClickHouse. Is that my understanding, correct?

@alexey-milovidov
Copy link
Member

No. We must support IPv6.

@alexey-milovidov
Copy link
Member

CURL_OSX_CALL_COPYPROXIES

This.

I don't know what is "copy proxies" on OS X, and so we don't need it.

@alexey-milovidov
Copy link
Member

Also please grep the code base for all instances of CURL_OSX_*.

@ghost
Copy link
Author

ghost commented Apr 4, 2022

Undefining this macro would require a code change in curl repo (curl/lib/curl_setup.h).

@alexey-milovidov
Copy link
Member

Another option is to disable curl, azure and sentry on Mac OS, so the problem will not exist.

@alexey-milovidov
Copy link
Member

Also getting rid of curl is a good option because we don't need it.
It can be done easily by implementing Azure::Core::Http::HttpTransport.

@ghost
Copy link
Author

ghost commented Apr 5, 2022

I'm planning to proceed with the option of disabling curl, azure and sentry on Mac OS.

@robot-ch-test-poll1 robot-ch-test-poll1 added the submodule changed At least one submodule changed in this PR. label Apr 6, 2022
@alexey-milovidov
Copy link
Member

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 7, 2022

update

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
DevTeamBK needs to authorize modification on its head branch.
err-code: 23BB7

@alexey-milovidov
Copy link
Member

This is ready to be merged.
Need to merge with master one more time.

@alexey-milovidov
Copy link
Member

Ok, let's try as is.

@alexey-milovidov alexey-milovidov merged commit 4691a42 into ClickHouse:master Apr 8, 2022
@azat
Copy link
Collaborator

azat commented Apr 8, 2022

Let's check the build log: https://s3.amazonaws.com/clickhouse-builds/35130/d19090fdaf89162ba5527f7f376394a6cdd4081f/binary_darwin/build_log.log

Looks like the problem with bool type.

@alexey-milovidov
Copy link
Member

ClickHouse build check (actions) — 22/22 builds are OK
ClickHouse special build check (actions) — 8/8 builds are OK

yokofly added a commit to timeplus-io/proton that referenced this pull request Oct 11, 2023
- Ported necessary changes from ClickHouse/ClickHouse#35130.
- Addressed compatibility post curl upgrade in commit ef02092.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants