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: don't use OpenGL in Qt on macOS #17676

Merged
merged 2 commits into from Dec 9, 2019
Merged

Conversation

@fanquake
Copy link
Member

fanquake commented Dec 5, 2019

Based on #17663. OpenGL on macOS was also deprecated in 10.14.

This also removes the /System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL dylib from bitcoin-qt.

fanquake added 2 commits Dec 3, 2019
This strips some unused dylibs from bitcoin-qt.

From man ld:
Remove dylibs that are unreachable by the entry point or exported symbols. 
That is, suppresses the generation of load command commands for dylibs 
which supplied no symbols during the link. This option should not be 
used when linking against a dylib which is required at runtime for 
some indirect reason such as the dylib has an important initializer.
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 6, 2019

Yes, OpenGL on MacOS is dead
ACK 2359a47

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 6, 2019

Gitian builds

File commit 6fff333
(master)
commit 2172174
(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz 94afa59905107ee2... f1d3764cd14d290e...
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz cbe33c3d4172b528... 21bcc99214d9ec10...
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz d091cac168218e0c... 7f0508d3aceaa667...
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz ebd3f5bb7f480a9d... aeec3f0b712e2094...
bitcoin-0.19.99-i686-pc-linux-gnu-debug.tar.gz 94c79aa5a768b4dc... 9c6d4f71012f7eda...
bitcoin-0.19.99-i686-pc-linux-gnu.tar.gz c559e31391f08655... 48346b5a9a00c8bf...
bitcoin-0.19.99-osx-unsigned.dmg 1ba5805f61cd56be... db05dbc6d4117605...
bitcoin-0.19.99-osx64.tar.gz 701efa7e02eb2599... 5db53e566dfad4c2...
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz b5aa4bedf8ab2858... 34b8be6a6ea320c1...
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz b3dec9053180e51f... b326906692f8f0a2...
bitcoin-0.19.99-win64-debug.zip adc71a666a2c99f1... 65f32120598c9949...
bitcoin-0.19.99-win64-setup-unsigned.exe e653d7edea605ec7... 2fdd8a1a5d967023...
bitcoin-0.19.99-win64.zip 89c9779f1f14b0cf... 7adaee4531110f44...
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz 7fafc74a26ca3ed9... c02346dbd25080b9...
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz 40e035c2ce1ab53a... 5b7ff35731e78af3...
bitcoin-0.19.99.tar.gz 1848ff553a1b601f... fb10f3361bef94f0...
bitcoin-core-linux-0.20-res.yml 0174c728bdc5acf3... 5a592f1742ee9651...
bitcoin-core-osx-0.20-res.yml c37690450571735b... a8a3e7d4125aa996...
bitcoin-core-win-0.20-res.yml 71a413f93902e13a... 4801b59c5301bf10...
linux-build.log 154db7314f7cbe08... d3db89f8f8505bd8...
osx-build.log 842bbcccf728796e... 22de28cee9fde388...
win-build.log 82825850e0c02173... 772df725332107eb...
bitcoin-core-linux-0.20-res.yml.diff b3778e9b866ec42e...
bitcoin-core-osx-0.20-res.yml.diff 5934d0cf7bee18a9...
bitcoin-core-win-0.20-res.yml.diff f8e41c2c896b6674...
linux-build.log.diff b493cefcb4f4618f...
osx-build.log.diff 9c86959f9abbbe7b...
win-build.log.diff dfa548204b2e6bf3...
@hebasto
hebasto approved these changes Dec 7, 2019
Copy link
Member

hebasto left a comment

ACK 2359a47

Ref: About OpenGL for OS X

Should contrib/macdeploy/macdeployqtplus be changed too?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 7, 2019

I don't think so, the only OpenGL related code there conditionally adds the OpenGL graphicssystem plugin:

            elif pluginPath == "graphicssystems/libqglgraphicssystem.dylib":
                # Deploy the opengl graphicssystem plugin only if QtOpenGL is in use
                if not deploymentInfo.usesFramework("QtOpenGL"):
                    continue

We don't use dynamically-loaded Qt plugins in the first place.

@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Dec 8, 2019

Should contrib/macdeploy/macdeployqtplus be changed too?

@laanwj is correct, nothing in that script needs to be changed.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Dec 9, 2019

Nice cleanup.
utACK 2359a47

laanwj added a commit that referenced this pull request Dec 9, 2019
2359a47 depends: don't use OpenGL in Qt on macOS (fanquake)
ba0cad2 build: pass -dead_strip_dylibs to ld on macOS (fanquake)

Pull request description:

  Based on #17663. OpenGL on macOS was also deprecated in 10.14.

  This also removes the `/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL` dylib from `bitcoin-qt`.

ACKs for top commit:
  laanwj:
    ACK 2359a47
  jonasschnelli:
    utACK 2359a47
  hebasto:
    ACK 2359a47

Tree-SHA512: 39b0151832c829f6ebdc4910eb28ebbeba64539cd04eba6ce3ec75fc0f231569956ca51a1e0bffc76dd27e85643c65a155320b9b450c49e9841e12b108406d41
@laanwj laanwj merged commit 2359a47 into bitcoin:master Dec 9, 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:macos_no_opengl branch Dec 9, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Dec 9, 2019
2359a47 depends: don't use OpenGL in Qt on macOS (fanquake)
ba0cad2 build: pass -dead_strip_dylibs to ld on macOS (fanquake)

Pull request description:

  Based on bitcoin#17663. OpenGL on macOS was also deprecated in 10.14.

  This also removes the `/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL` dylib from `bitcoin-qt`.

ACKs for top commit:
  laanwj:
    ACK 2359a47
  jonasschnelli:
    utACK 2359a47
  hebasto:
    ACK 2359a47

Tree-SHA512: 39b0151832c829f6ebdc4910eb28ebbeba64539cd04eba6ce3ec75fc0f231569956ca51a1e0bffc76dd27e85643c65a155320b9b450c49e9841e12b108406d41
fanquake added a commit to fanquake/core-review that referenced this pull request Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.