Skip to content

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Mar 1, 2021

Currently, our cross-compile of libnatpmp for macOS doesn't work at all.
The wrong archiver is used, which produces an archive the linker doesn't like.
This becomes clear when configuring:

configure:25722: checking for initnatpmp in -lnatpmp
configure:25747: env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/bitcoin/depends/x86_64-apple-darwin18/native/bin/clang++ --target=x86_64-apple-darwin18 <trim>  -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-dead_strip_dylibs conftest.cpp -lnatpmp   >&5
ld: archive has no table of contents for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Fix this by using the right ar (we do the same for upnp).

While we're at it, fix the build so that we are using our c/ppflags.
In practice this basically means building with -O2 rather than -Os.

Note that this fixes an issue that is also fixed by #21209. However, given there are reservations about updating to use a newer libnatpmp source, we should just fix this for now.

Currently, our cross-compile of libnatpmp for macOS doesn't work at all.
The wrong archiver is used, which produces an archive the linker doesn't like.
This becomes clear when configuring:
```bash
configure:25722: checking for initnatpmp in -lnatpmp
configure:25747: env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/bitcoin/depends/x86_64-apple-darwin18/native/bin/clang++ --target=x86_64-apple-darwin18 <trim>  -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-dead_strip_dylibs conftest.cpp -lnatpmp   >&5
ld: archive has no table of contents for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

Fix this by using the right `ar` (we do the same for upnp).

While we're at it, we fixe the build so that we are using our c/ppflags.
This  means building with `-O2` rather than `-Os`.

Note that this fixes an issue that is also fixed by bitcoin#21209.
However, given there are reservations about updating to use a newer libnatpmp source, we should just fix this for now.
@hebasto
Copy link
Member

hebasto commented Mar 1, 2021

Concept ACK.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 1, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK bd49ac4, tested:

  • cross-compiling -- on Linux Mint 20.1 (x86_64)
  • binaries -- on macOS Big Sur 11.2.1 (20D74)

Cross-compiling:

$ CONFIG_SITE=$PWD/depends/x86_64-apple-darwin16/share/config.site ./configure
...
Options used to compile and link:
...
  with natpmp     = no
$ CONFIG_SITE=$PWD/depends/x86_64-apple-darwin16/share/config.site ./configure
...
Options used to compile and link:
...
  with natpmp     = yes

Replacement compiler flags s /Os/O2/ could increase code size by using more optimization options. I'm ok with it.

Also added the -pipe flag which has no effect on the generated code at all, only on compiling speed.

@hebasto
Copy link
Member

hebasto commented Mar 1, 2021

Backport to 0.21 ?

@fanquake
Copy link
Member Author

fanquake commented Mar 2, 2021

Backport to 0.21 ?

Why? libnatpmp isn't in the 0.21 branch.

@hebasto
Copy link
Member

hebasto commented Mar 2, 2021

Why? libnatpmp isn't in the 0.21 branch.

Indeed. I messed my memory. Sorry for noise.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 2, 2021

Guix builds

File commit ad89812
(master)
commit f8821ec
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 580b0205e5f6ab95... 6d912ef0a0f2a06b...
*-aarch64-linux-gnu.tar.gz 03136fb8a2ce5f88... eff1114521173432...
*-arm-linux-gnueabihf-debug.tar.gz 053f6f1bcb7c905c... 332d0f142feb4ad7...
*-arm-linux-gnueabihf.tar.gz 996df42a3657a608... 8bcb682677c350f8...
*-osx-unsigned.dmg 50694aa5dbc88eac... facafa690ce23f66...
*-osx-unsigned.tar.gz 944075ed83089652... 5b1939a89cb6680b...
*-osx64.tar.gz b6a084a966bee2f3... f6d4f89169f72586...
*-powerpc64-linux-gnu-debug.tar.gz 7796ea50573078f7... f8bf06814e1993b6...
*-powerpc64-linux-gnu.tar.gz e00672d7411277b3... ff0a88d1972d59d1...
*-powerpc64le-linux-gnu-debug.tar.gz d8d73f5d2ddf14f4... 89d4b4f8eac0fabc...
*-powerpc64le-linux-gnu.tar.gz 0a9826f7a9526c9d... 3d3cb598ad881b75...
*-riscv64-linux-gnu-debug.tar.gz f4875638f5083b34... 846a3b9805ab47f5...
*-riscv64-linux-gnu.tar.gz 67500503434aeb02... ff39654cf64ed45c...
*-win-unsigned.tar.gz 16c5b021d4b012f7... fd3f415e86025a3f...
*-win64-debug.zip 2b448f2e34cc993f... 914f34e0c01cd37c...
*-win64-setup-unsigned.exe 6d9b4d8dd77e886a... e174f622aad0fe40...
*-win64.zip 9369f87ed5e94646... f7dbf7a7b7c54cb6...
*-x86_64-linux-gnu-debug.tar.gz e82361425e5cf739... ff1f39905dc48671...
*-x86_64-linux-gnu.tar.gz 17deed2695cc41c5... 46a81edd8b3b8a9f...
*.tar.gz 36675469b1db5989... 9ccb7c587f348cbf...
guix_build.log ae0fc0b2f1615242... 7e66f2d3f9a92ca8...
guix_build.log.diff 5929bef20c26a83b...

@fanquake fanquake merged commit 97a35f3 into bitcoin:master Mar 3, 2021
@fanquake fanquake deleted the fix_libnatpmp_cross_macos branch March 3, 2021 12:04
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 3, 2021
bd49ac4 build: fix libnatpmp macos cross compile (fanquake)

Pull request description:

  Currently, our cross-compile of libnatpmp for macOS doesn't work at all.
  The wrong archiver is used, which produces an archive the linker doesn't like.
  This becomes clear when configuring:
  ```bash
  configure:25722: checking for initnatpmp in -lnatpmp
  configure:25747: env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/bitcoin/depends/x86_64-apple-darwin18/native/bin/clang++ --target=x86_64-apple-darwin18 <trim>  -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-dead_strip_dylibs conftest.cpp -lnatpmp   >&5
  ld: archive has no table of contents for architecture x86_64
  clang: error: linker command failed with exit code 1 (use -v to see invocation)
  ```

  Fix this by using the right `ar` (we do the same for upnp).

  While we're at it, fix the build so that we are using our c/ppflags.
  In practice this basically means building with `-O2` rather than `-Os`.

  Note that this fixes an issue that is also fixed by bitcoin#21209. However, given there are reservations about updating to use a newer libnatpmp source, we should just fix this for now.

ACKs for top commit:
  hebasto:
    ACK bd49ac4, tested:

Tree-SHA512: 2efc2c788ef3ebebfbf564ef07b6cf63a72d8a0bccc22b0ba36537216aa575436b7e87088477e85f6d9191ad34f0b13f1c22cf88c90e1cb81641bfee5dc3058a
kwvg added a commit to kwvg/dash that referenced this pull request Feb 26, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants