Page MenuHomeWildfire Games

GnuTLS configure requires pkg-config, breaking macOS build
ClosedPublic

Authored by historic_bruno on Jul 9 2019, 8:42 PM.

Details

Reviewers
trompetin17
Group Reviewers
macOS Developers
Trac Tickets
#5453
#5481
#5489
#5503
Summary

On macOS, without a 3rd party package manager installed (e.g. Homebrew), libraries/osx/build-osx-libs.sh fails with the following error:

checking for NETTLE... configure: error: 
  ***
  *** Libnettle 3.1 was not found.
ERROR: GnuTLS build failed

This is the result of GnuTLS using pkg-config to check for nettle, hogweed, and other dependencies. Since there is no built-in option for overriding this behavior, this patch simply removes those error cases, using our custom configure flags instead.

Test Plan
  1. Run libraries/osx/build-osx-libs.sh --force-rebuild on a macOS system without pkg-config (or unset PKG_CONFIG or remove it from path temporarily)

NOTE: it's possible to rebuild only the affected libs to save time:

rm libraries/osx/{gloox,gmp,gnutls,nettle}/.already-built
libraries/osx/build-osx-libs.sh
  1. Build should finish successfully
  2. Run game and connect to multiplayer lobby with TLS enabled
  3. Should connect to lobby and function as expected (chat, join/host games, etc)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

historic_bruno created this revision.Jul 9 2019, 8:42 PM
historic_bruno planned changes to this revision.Jul 9 2019, 9:05 PM
elexis added a subscriber: elexis.Jul 9 2019, 9:17 PM

(See discussion on #0ad-dev, it will need an update for 3.6 and the 3.6 update ought to be before a release. The patch is intended for the current 3.5)

Updates nettle to 3.5.1, gnutls to 3.6.8, and gloox to 1.0.22.
Fixes GnuTLS build by adding gmp to the LIBS var, otherwise it's a missing dependency in the configure script (they only add NETTLE_LIBS and HOGWEED_LIBS). NOTE: Upstream should include GMP_LIBS in the configure script.
GnuTLS 3.6.8 implements a new feature TCP Fast Open which requires a system function connectx, not available until OS X 10.11. Unfortunately GnuTLS doesn't currently support SDK-based builds, so the feature must either be enabled or disabled. I have chosen to disable it on macOS, as it's only an optional feature. We could certainly revisit an upstream patch to provide proper macOS SDK-based builds, or at least a build flag to disable TCPFO.

Also:
Simplifies .gitignore so we don't have to keep adding exclusions for every new macOS library.
Removes some unnecessary build flags in build-osx-libs.sh.

For a look at what disabled TCP fast open means, see https://gitlab.com/gnutls/gnutls/blob/master/lib/system/fastopen.c

It falls back to using simple connect(), the same as for "unsupported platforms".

Diff with full context

elexis added inline comments.Jul 10 2019, 3:19 PM
libraries/osx/build-osx-libs.sh
492

Why does this revert rP22212?

Actually I don't know why the flag was added:
From https://code.wildfiregames.com/D1772#75868

You should ask Stan if you want to know how he later pinpointed the flag as solving the bug.

https://gmplib.org/manual/Build-Options.html
Using --enable-fat selects a “fat binary” build on x86, where optimized low level subroutines are chosen at runtime according to the CPU detected. This means more code, but gives good performance on all x86 chips. (This option might become available for more architectures in the future.)

historic_bruno planned changes to this revision.Jul 10 2019, 4:32 PM
historic_bruno added inline comments.
libraries/osx/build-osx-libs.sh
492

Good catch, it should not be removed. I updated D1772 with explanation of why, and I will add some documentation to the script about why it's needed (build-time detection of CPU-specific instructions). Also need to check other libs for this potential problem -- nettle can enable fat binaries too.

historic_bruno edited the test plan for this revision. (Show Details)

Restore --enable-fat for GMP, and add to nettle as well.

From D1772#85808:

Encountered this today while working on D2057. I mistakenly removed the flag, thinking it was only macOS "fat binaries" which are combined x86_64 and i386 archs.
In fact, it's more significant for libraries like GMP, because it detects CPU-specific instructions during the build process. If you build on a new CPU, and try to run on an older CPU, there's a good chance it will crash. I'm sure that's what happened before. Here's an example: https://gmplib.org/list-archives/gmp-bugs/2019-May/004549.html
I'm adding a note in the script about why the flag is required now.

historic_bruno updated this revision to Diff 8828.EditedJul 11 2019, 2:17 AM

GnuTLS LIB_URL update was omitted from previous diff.

I noticed that gloox 1.0.22 enables getaddrinfo by default, where it was only a build option before. So I've added --disable-getaddrinfo to maintain the previous behavior, until we can test the consequences of this change wrt/ IPv6 and various user network configurations.

historic_bruno updated the Trac tickets for this revision.

While I'm here:
Add --with-pic to GMP configure flags, fixing ld warning reported in #5489.

@historic_bruno I think this patch worked for me as I was able to build a24 without any errors. However, when I try to login to the multiplayer lobby it says "Resolving the server's hostname failed". Is that the expected result?

@historic_bruno I think this patch worked for me as I was able to build a24 without any errors. However, when I try to login to the multiplayer lobby it says "Resolving the server's hostname failed". Is that the expected result?

I have the same error with my brew setup, which uses GNUTLS 3.6 also. There might be something there.

@historic_bruno I think this patch worked for me as I was able to build a24 without any errors. However, when I try to login to the multiplayer lobby it says "Resolving the server's hostname failed". Is that the expected result?

I had that error before adding the --disable-getaddrinfo flag to the gloox build. Are you sure you rebuilt gloox and used the latest diff?

historic_bruno edited the test plan for this revision. (Show Details)Jul 11 2019, 8:15 PM

@historic_bruno The patch worked for me :) I was able to connect to the a24 lobby successfully.

Running binaries/system/test I did get:

Running cxxtest tests (322 tests).....................................................................................................................................................................................
In TestCStr::test_parse:
/Users/ever/dev/games/0ad/source/ps/tests/test_CStr.h:135: Error: Expected (str3.ToFloat() == 3.0f), found (0.0000 != 3.0000)
/Users/ever/dev/games/0ad/source/ps/tests/test_CStr.h:136: Error: Expected (str3.ToDouble() == 3.0), found (0.0000 != 3.0000)
............................................................................................................................................
Failed 1 and Skipped 0 of 322 tests
Success rate: 99%

But I'm not sure if that's related to the patch or not. Either way, thanks for working on this! I think I'm good to go now.

trompetin17 accepted this revision.Jul 11 2019, 10:49 PM
trompetin17 added a subscriber: trompetin17.

tested in mojave

This revision is now accepted and ready to land.Jul 11 2019, 10:49 PM
historic_bruno updated the Trac tickets for this revision.Jul 13 2019, 7:41 PM
historic_bruno added inline comments.Jul 17 2019, 6:02 AM
.gitignore
58

Note: I had a change of mind about this. I don't like excluding everything by default, better to manage the list of subdirectories. The reason: if someone adds new source controlled files in libraries/osx, it would be too easy to forget them on commit.

So the actual change will be adding:

libraries/osx/gmp
libraries/osx/gnutls
libraries/osx/libsodium
libraries/osx/nettle