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

depends: remove Qt networking features #17730

Merged
merged 3 commits into from Dec 16, 2019

Conversation

@fanquake
Copy link
Member

fanquake commented Dec 12, 2019

Somewhat of a followup to removing BIP70 support in #17165. This removes networking features from our Qt build. This also removes the need to link against the CFNetwork and SystemConfiguration libraries on macOS.

src/qt/bitcoin-qt:
 /usr/lib/libSystem.B.dylib 
 /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit 
 /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation 
 /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices 
 /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit 
 /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices 
 /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation 
-/System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration 
 /System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics 
 /System/Library/Frameworks/Carbon.framework/Versions/A/Carbon 
 /usr/lib/libc++.1.dylib 
-/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork 
 /System/Library/Frameworks/CoreText.framework/Versions/A/CoreText 
 /System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO 
 /usr/lib/libobjc.A.dylib 

Introduced the -optimized-tools option; supersedes -optimized-qmake.

optimized-qmake became optimized-tools in Qt 5.6.0. While the former still works, we can use the newer flag.

A diff of the removed symbols is available here.

We still need to actually build the network module, because we are using QLocalServer & QLocalSocket in the payment server.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 12, 2019

Concept ACK

We still need to actually build the network module, because we are using QLocalServer & QLocalSocket in the payment server.

Yes,, this is the only cross-platform IPC mechanism that Qt has, afaik (it was at the time of 4.x).

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 12, 2019

Concept ACK

Tested 58f9f23 on macOS 10.15.2, was able to create a DMG and BIP20 URLs can still be opened.

@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Dec 12, 2019

Annoyingly, the Windows build blows up without networkinterface. Looking for a workaround.

@fanquake fanquake force-pushed the fanquake:qt_no_lib_system_proxy branch from 58f9f23 to 3569e0e Dec 12, 2019
@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Dec 12, 2019

This is now ready for review. I've fixed the Windows build by reinstating the networkinterface feature.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 13, 2019

Tested ACK 3569e0e on macOS 10.15.2. I would like to see this tested on Windows and work on a repaired AppVeyor though.

fanquake added 3 commits Dec 5, 2019
@fanquake fanquake force-pushed the fanquake:qt_no_lib_system_proxy branch from 3569e0e to 244501f Dec 13, 2019
@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Dec 13, 2019

work on a repaired AppVeyor though.

Rebased on master for Appveyor.

@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Dec 13, 2019

@MarcoFalke the bot built the old branch. That error is no longer relevant.

@bitcoin bitcoin deleted a comment from DrahtBot Dec 13, 2019
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 13, 2019

Code review ACK 244501f: just a rebase (updated since I accidentally repeated the previous hash)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 14, 2019

ACK 244501f -- diff looks correct

Thanks for doing these cleanups

@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 14, 2019

Code review ACK 244501f.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 14, 2019

Gitian builds

File commit c5e318a
(master)
commit 261d979
(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz bea40f9a3e1e3d15... 1a1468d0438c548a...
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz 6062abc6a838a5a6... 39d213b6222d94c0...
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz 69dcc7a49e222ca9... 581789bae808dd76...
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz d0e369041d9fe9b5... 66f141b937fa43bb...
bitcoin-0.19.99-i686-pc-linux-gnu-debug.tar.gz d3b7c55ff3c2bc2f... dffbfae69f7b596c...
bitcoin-0.19.99-i686-pc-linux-gnu.tar.gz ae365922d91820b9... 48a8dc0f0533338c...
bitcoin-0.19.99-osx-unsigned.dmg d5388aa2474ac73e... 158abaec2dc845e9...
bitcoin-0.19.99-osx64.tar.gz 4baf8f214f059caf... 2df7fa0bc8034c1f...
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz 6b0e20d11cb5fe6b... 5d7055cc3f5c88f8...
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz 0d45828e7aac5e93... 50bda054dd366e9c...
bitcoin-0.19.99-win64-debug.zip 5669c48e3fad4bc5... 238ae11e10578ff6...
bitcoin-0.19.99-win64-setup-unsigned.exe 6fc70664b2973325... aea2f18ecf4cba4b...
bitcoin-0.19.99-win64.zip 576846961cf26ff2... 3f1bc9e0963f3c53...
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz 591688b38863b8b4... 6a7cc40d96ada4d5...
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz e8dcc933264b4d6c... c449874653133164...
bitcoin-0.19.99.tar.gz 9d45221fe86cb679... f4183c4fdbca951d...
bitcoin-core-linux-0.20-res.yml 2efef3cbffb806a2... d9c82c697f8efefd...
bitcoin-core-osx-0.20-res.yml d2ea6d1607e9fd97... af5b9d36f91aed00...
bitcoin-core-win-0.20-res.yml 93ecd38034110860... 88524bd7b65572f9...
linux-build.log b3d2ce067eb93baa... 2139e073d3ac6578...
osx-build.log f3e9977f0c38cddd... 63fe092e173a129b...
win-build.log 91466d2d19dec4e6... 4de24548768d0858...
bitcoin-core-linux-0.20-res.yml.diff 59307791323c198e...
bitcoin-core-osx-0.20-res.yml.diff f2f7ee5deb06c382...
bitcoin-core-win-0.20-res.yml.diff ecea253444bc8d99...
linux-build.log.diff fabfe4a4d8683e7c...
osx-build.log.diff 35eb01436ac7a0ba...
win-build.log.diff 9ac7e7efd0559d47...
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 15, 2019

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

Conflicts

No conflicts as of last run.

laanwj added a commit that referenced this pull request Dec 16, 2019
244501f depends: disable unused qt networking features (fanquake)
29d56c6 depends: -optimized-qmake is now -optimized-tools (fanquake)
ccdda96 depends: skip building qt proxies (fanquake)

Pull request description:

  Somewhat of a followup to removing BIP70 support in #17165. This removes networking features from our Qt build. This also removes the need to link against the `CFNetwork` and `SystemConfiguration` libraries on macOS.

  ```diff
  src/qt/bitcoin-qt:
   /usr/lib/libSystem.B.dylib
   /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
   /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
   /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices
   /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
   /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
   /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  -/System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
   /System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics
   /System/Library/Frameworks/Carbon.framework/Versions/A/Carbon
   /usr/lib/libc++.1.dylib
  -/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork
   /System/Library/Frameworks/CoreText.framework/Versions/A/CoreText
   /System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO
   /usr/lib/libobjc.A.dylib
  ```

  > Introduced the -optimized-tools option; supersedes -optimized-qmake.

  `optimized-qmake` became `optimized-tools` in Qt 5.6.0. While the former still works, we can use the newer flag.

  A diff of the removed symbols is available [here](https://gist.github.com/fanquake/9c8d5961c91f90a2966191367adfb391).

  We still need to actually build the network module, because we are using `QLocalServer` & `QLocalSocket` in the payment server.

ACKs for top commit:
  Sjors:
    Code review ACK 244501f: just a rebase (_updated since I accidentally repeated the previous hash_)
  practicalswift:
    ACK 244501f -- diff looks correct
  promag:
    Code review ACK 244501f.

Tree-SHA512: 79734e3c96c40e7e484c86ac4cd4f738c05fcebe4771aeac443883f618a6c766e667909d5f8f14f9bd82f43206387c952458c5fa765cd0830f8beda6e6ac80ae
@laanwj laanwj merged commit 244501f into bitcoin:master Dec 16, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake deleted the fanquake:qt_no_lib_system_proxy branch Dec 16, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Dec 16, 2019
244501f depends: disable unused qt networking features (fanquake)
29d56c6 depends: -optimized-qmake is now -optimized-tools (fanquake)
ccdda96 depends: skip building qt proxies (fanquake)

Pull request description:

  Somewhat of a followup to removing BIP70 support in bitcoin#17165. This removes networking features from our Qt build. This also removes the need to link against the `CFNetwork` and `SystemConfiguration` libraries on macOS.

  ```diff
  src/qt/bitcoin-qt:
   /usr/lib/libSystem.B.dylib
   /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
   /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
   /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices
   /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
   /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
   /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  -/System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
   /System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics
   /System/Library/Frameworks/Carbon.framework/Versions/A/Carbon
   /usr/lib/libc++.1.dylib
  -/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork
   /System/Library/Frameworks/CoreText.framework/Versions/A/CoreText
   /System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO
   /usr/lib/libobjc.A.dylib
  ```

  > Introduced the -optimized-tools option; supersedes -optimized-qmake.

  `optimized-qmake` became `optimized-tools` in Qt 5.6.0. While the former still works, we can use the newer flag.

  A diff of the removed symbols is available [here](https://gist.github.com/fanquake/9c8d5961c91f90a2966191367adfb391).

  We still need to actually build the network module, because we are using `QLocalServer` & `QLocalSocket` in the payment server.

ACKs for top commit:
  Sjors:
    Code review ACK 244501f: just a rebase (_updated since I accidentally repeated the previous hash_)
  practicalswift:
    ACK 244501f -- diff looks correct
  promag:
    Code review ACK 244501f.

Tree-SHA512: 79734e3c96c40e7e484c86ac4cd4f738c05fcebe4771aeac443883f618a6c766e667909d5f8f14f9bd82f43206387c952458c5fa765cd0830f8beda6e6ac80ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.