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

Update libcurl #5892

Merged
merged 11 commits into from
Dec 6, 2024
Merged

Update libcurl #5892

merged 11 commits into from
Dec 6, 2024

Conversation

Redbeanw44602
Copy link
Contributor

No description provided.

@Redbeanw44602
Copy link
Contributor Author

Reasons for the error on apple platform: curl/curl#14269

@SirLynix
Copy link
Member

SirLynix commented Dec 3, 2024

we can add curl/curl@5f6b924 as a patch for versions where it fails

@Redbeanw44602
Copy link
Contributor Author

we can add curl/curl@5f6b924 as a patch for versions where it fails

Thanks, I'm doing this.

@Redbeanw44602
Copy link
Contributor Author

I think it's ready to be merged now.

@star-hengxing star-hengxing requested a review from waruqi December 6, 2024 07:06
@waruqi waruqi merged commit a23c2b6 into xmake-io:dev Dec 6, 2024
Comment on lines -35 to -37
if package:is_plat("linux", "android", "cross") then
package:config_set("openssl", true)
end
Copy link
Member

Choose a reason for hiding this comment

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

Removing those lines broke existing xmake projects doing add_requires("libcurl")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works fine on my device (linux, x86_64), can you provide more information?

Copy link
Member

Choose a reason for hiding this comment

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

add_requires("libcurl") on Linux no longer links with libcurl and no longer has HTTPS support, resulting in unsupported protocol errors.

before your change, openssl support was enabled by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does break the default HTTPS support, but this is expected because

  1. HTTPS support should not be forced, curl builds do not enforce ssl
  2. There are other options now (openssl3)

Downstreams should choose whether to enable encryption, and which one to enable.

Copy link
Contributor Author

@Redbeanw44602 Redbeanw44602 Dec 13, 2024

Choose a reason for hiding this comment

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

If one must be turned on one ssl backend by default, it is difficult to choose which one to turn on. I think it is best to let the user choose.

Copy link
Member

Choose a reason for hiding this comment

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

I understand it's expected, but it's problematic to update a package and break the existing behavior, especially here where all xmake projects using libcurl will get a failure on the broadly used https protocol.

openssl should still be the default when no option is set, I think we can detect the case where openssl and openssl3 are not set (nil) and enable openssl as before for retrocompatibility.

@waruqi is this possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the ability to build libcurl without SSL support needs to be preserved, is there a way for xmake to tell if a certain config is set "by default"? Otherwise I think it's a good idea to automatically enable openssl.

Copy link
Member

@SirLynix SirLynix Dec 13, 2024

Choose a reason for hiding this comment

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

I think so but I need confirmation from xmake author, afaik configs can have three values : true, false or nil (not set).
what I'm suggesting is basically:

if package:config("openssl") == nil and package:config("openssl3") == nil then
    package:config_set("openssl", true)
end

this will support add_requires("libcurl", { configs = { openssl = false } }) as well

Copy link
Member

Choose a reason for hiding this comment

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

If openssl is not explicitly set and no default value is configured in add_configs, it defaults to nil

SirLynix added a commit to NazaraEngine/NazaraEngine that referenced this pull request Dec 13, 2024
following this package update: xmake-io/xmake-repo#5892 which disabled openssl by default
SirLynix added a commit that referenced this pull request Dec 13, 2024
SirLynix added a commit to DigitalPulseSoftware/ThisSpaceOfMine that referenced this pull request Dec 13, 2024
waruqi pushed a commit that referenced this pull request Dec 14, 2024
* libcurl: Restore openssl by default on Linux/Android

See #5892

* Update xmake.lua
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.

4 participants