HomeWildfire Games

Update macOS libcurl --without-libidn flag to --without-libidn2 following https…
AuditedrP21913

Description

Update macOS libcurl --without-libidn flag to --without-libidn2 following https://github.com/curl/curl/commit/9c91ec778104ae3b744b39444d544e82d5ee9ece

Fixes #5231.
Refs: rP19848, rP21683.
Differential Revision: https://code.wildfiregames.com/D1610
Patch By: Tobbi and viky / Victor Adascalitei
Comments By: Itms, andy5995

Details

Auditors
Itms
Committed
elexisOct 19 2018, 6:14 PM
Differential Revision
D1610: build-osx-libs.sh: add --without-libidn2 flag to curl region
Parents
rP21912: [i18n] Updated POT and PO files.
Branches
Unknown
Tags
Unknown
Build Status
Buildable 6398
Build 10600: Post-Commit BuildJenkins

Event Timeline

Itms awarded a token.Oct 20 2018, 12:07 PM
Itms raised a concern with this commit.Oct 23 2018, 11:08 PM
Itms added a subscriber: Itms.

It looks like this commit breaks mod.io on macOS (no https support anymore, whereas it worked on the previous revision), but I can't understand why.

This commit now has outstanding concerns.Oct 23 2018, 11:08 PM
  • What is the error message?
  • Sure it is this commit and not rP21914, or a forgotton cleaning of buildfiles, if so how do you know?
Itms added a comment.Oct 24 2018, 11:15 AM
  • What is the error message?
  • Sure it is this commit and not rP21914, or a forgotton cleaning of buildfiles, if so how do you know?

The error message is something along the lines of "https protocol not supported by curl". It could be rP21914 indeed, I tested on the previous github merge commit, not in between. But I assumed it was this specific commit because of the error message. I tested by rebuilding the libraries, sure.
My understanding of this issue is that the new combination of flags somehow makes configure believe we are not asking for ssl support. But Tobbi did build the A23 release with this and nobody complained about modio not working, so I'm a bit puzzled.

To be honest I raised an issue in case somebody can reproduce it and propose a fix, but I have not finished looking into this issue. I should be able to troubleshoot it, or at least understand the real issue, when I can spend some time again on my VM (probably this weekend, I'll be using my laptop until then). If macOS SVN users don't encounter the problem, I'll just track down why my test failed, and remove my Concern.

elexis requested verification of this commit.Oct 24 2018, 11:33 AM
In rP21913#31474, @Itms wrote:

The error message is something along the lines of "https protocol not supported by curl".
My understanding of this issue is that the new combination of flags somehow makes configure believe we are not asking for ssl support.

It does ask for ssl since it complains about ssl not being available. (It should ask for ssl if and only if the default.cfg mod.io url starts with https://)

Tobbi did build the A23 release with this

I confirm mod.io does work with macOS release, as I asked HMS-Surprise to test that a week ago.
But Tobbi did build with --without-libidn --without-libidn2 but in this commit I removed --without-libidn since the flag does not exist anymore according to my research (see my comments in rP21683).

To be honest I raised an issue in case somebody can reproduce it and propose a fix, but I have not finished looking into this issue. I should be able to troubleshoot it, or at least understand the real issue, when I can spend some time again on my VM (probably this weekend, I'll be using my laptop until then). > If macOS SVN users don't encounter the problem, I'll just track down why my test failed, and remove my Concern.

(I don't know any macOS users who compile.)

I really think this should not be related to this commit, because in theory IDN means internationalized domainname support and mod.io is not an internationalized domainname and it doesn't complain about domainnames and in since it's not demonstrated to be this commit.

Perhaps it's libCURL trying but failing to use GnuTLS.

This commit now requires verification by auditors.Oct 24 2018, 11:33 AM
Stan added a subscriber: Stan.Oct 24 2018, 11:37 AM

I'm pretty sure we can't have letters in mod versions.

Itms added a comment.Oct 24 2018, 12:02 PM

It does ask for ssl since it complains about ssl not being available. (It should ask for ssl if and only if the default.cfg mod.io url starts with https://)

I was talking about the compilation of curl, not the request made to mod.io.

I confirm mod.io does work with macOS release, as I asked HMS-Surprise to test that a week ago.
But Tobbi did build with --without-libidn --without-libidn2 but in this commit I removed --without-libidn since the flag does not exist anymore according to my research (see my comments in rP21683).

Yeah I know, I'm just thinking out loud.

I really think this should not be related to this commit, because in theory IDN means internationalized domainname support and mod.io is not an internationalized domainname and it doesn't complain about domainnames and in since it's not demonstrated to be this commit.

Yeah I agree this commit is not supposed to break anything...

Perhaps it's libCURL trying but failing to use GnuTLS.

Very unlikely (it has no way to know there is a new gnutls library compiled in a neighboring directory), but still possible. I'll really have to test rP21913 and rP21914 from scratch.

Perhaps it's libCURL trying but failing to use GnuTLS.
it has no way to know there is a new gnutls library compiled in a neighboring directory), but still possible.

If GnuTLS was coped to some /usr system directory and if libCURL would search for the given and /usr system directories, it might.
libCURL doesn't receive the --without-gnutls argument and according to https://github.com/curl/curl/commit/b180a273fab64b601e7f44a36a0ec8bd5363e8e1 libCURL has the ability to build with multiple backends.
According to https://ec.haxx.se/building-tls.html it will detect GnuTLS in its default path by default. You can optionally point configure to a custom install path prefix where it can find gnutls.
gloox in D1654 also uses --with-gnutls="yes", indicating use of a system directory for GnuTLS.
Anyway, speculation, needs testing.

Itms accepted this commit.Oct 31 2018, 6:57 PM

After a more careful testing, indeed this commit doesn't introduce the issue. It might come from D1654 after all. However,

According to https://ec.haxx.se/building-tls.html it "will detect GnuTLS in its default path by default. You can optionally point configure to a custom install path prefix where it can find gnutls".

Yes, but gnutls hasn't been installed in a default path, and I didn't ask curl to use it.

gloox in D1654 also uses --with-gnutls="yes", indicating use of a system directory for GnuTLS.

No, -with-gnutls in gloox only accepts yes and no, not custom prefixes like it should. I manually added the gnutls locations to the build and link flags.

Further proof of the fact that curl didn't use system libraries is that cleaning the git repo was enough to make mod.io work again on rP21913.

libCURL doesn't receive the --without-gnutls argument

Yes, this is probably the way to fix the issue if D1654 is the cause after all.

All concerns with this commit have now been addressed.Oct 31 2018, 6:57 PM