qt58: enable darwin compatibility #24139

periklis · 2017-03-20T21:13:56Z

Motivation for this change

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-03-20T21:13:57Z

@periklis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ttuegel, @bjornfor and @orivej to be potential reviewers.

orivej · 2017-03-21T03:12:27Z

pkgs/development/libraries/qt-5/5.8/qtbase/default.nix

@@ -15,7 +17,8 @@
  cups ? null, mysql ? null, postgresql ? null,

  # options
-  mesaSupported, mesa,
+  mesaSupported ? (!stdenv.isDarwin),


Why would the default value have any difference when mesaSupported is defined in all-packages.nix? And why would you want to disable mesa on Darwin?

afaik, mesa is supported for darwin, but the corresponding mkspecs-libgl.path has to be ported for darwin, too. I assume that the following defs in `mkspecs/common/mac.conf' have to be substituted by the mesa-buildInput:

QMAKE_LIBDIR = # sdk.prf will prefix the proper SDK sysroot QMAKE_INCDIR_OPENGL = \ /System/Library/Frameworks/OpenGL.framework/Headers \ /System/Library/Frameworks/AGL.framework/Headers/

Since this is an issue with all qt-5 releases, i would like to suggest a separate PR. What is your opinion?

mesaSupported and mesaPlatforms are just bad names. On Darwin we do not support mesa itself, but there's the libGL* from the OS that we do support, provided at the same mesa_noglu attribute.

Thx for the insights @vcunat . I still propose to re-enable mesa/libGL support in a separate PR for the series of currently maintained qt-5 release. What do you think?

Yes, it seems OK to go without mesa support here for now.

orivej · 2017-03-21T03:18:34Z

pkgs/development/libraries/qt-5/5.8/qtbase/default.nix

+          ./mkspecs/common/clang-mac.conf
+     '';
+     # Note on the above: \x27 is a way if including a single-quote
+     # character in the sed string arguments.


There is a typo in a way if including, but a patch (plus some substituteInPlaces) would be much easier to review and maintain for future Qt releases than this sequence of seds.

Sure, i adopted for the start the sed-way, because of it was the natural regex-replace strategy. Does substituteInPlace know regex?

orivej · 2017-03-21T03:20:13Z

pkgs/development/libraries/qt-5/5.8/qtbase/default.nix

@@ -102,7 +125,7 @@ stdenv.mkDerivation {
    -shared
    ${lib.optionalString developerBuild "-developer-build"}
    -accessibility
-    -rpath
+    -optimized-qmake


Where does this -optimized-qmake come from and what does it do?

Afaik this configure option is for cross-compilation, e.g. raspeberry-pi, etc.

I suspect following reasoning

Shrink the temp directory used for building with qmake.

according to 7a9400d

orivej · 2017-03-21T03:20:34Z

pkgs/development/libraries/qt-5/5.8/qtbase/default.nix

@@ -155,40 +164,75 @@ stdenv.mkDerivation {
    -${lib.optionalString (buildExamples == false) "no"}make examples
    -${lib.optionalString (buildTests == false) "no"}make tests
    -v
-  '';
+  '' + lib.optionalString (!stdenv.isDarwin) ''
+    -no-rpath


You replaced -rpath with -no-rpath.

orivej · 2017-03-21T03:21:25Z

pkgs/development/libraries/qt-5/5.8/qtbase/default.nix


    # Don't move .prl files on darwin because they end up in
    # "dev/lib/Foo.framework/Foo.prl" which interferes with subsequent
    # use of lndir in the qtbase setup-hook. On Linux, the .prl files
    # are in lib, and so do not cause a subsequent recreation of deep
    # framework directory trees.
-    + lib.optionalString stdenv.isDarwin ''
+    '' + lib.optionalString stdenv.isDarwin ''


Why move the quotes around here and 9 lines above?

orivej · 2017-03-21T03:24:07Z

pkgs/development/libraries/qt-5/5.8/qtbase/default.nix

@@ -266,7 +307,7 @@ stdenv.mkDerivation {
    description = "A cross-platform application framework for C++";
    license = with licenses; [ fdl13 gpl2 lgpl21 lgpl3 ];
    maintainers = with maintainers; [ bbenoist qknight ttuegel ];


Wouldn't you like to ensure being notified of future proposed changes to qtbase by listing yourself as a maintainer?

periklis · 2017-03-21T07:12:59Z

@orivej Thx again for your time and thorough review. Fyi, this patch is a port of the similar patch for qt-5.6 (7a9400d, intial by @acowley) and porting it by me for qt-5.7 (#23602, #23764). However, atm i ported it to qt-5.7, 5.8 got pushed into staging :)

periklis · 2017-03-22T15:32:30Z

fyi, qt 5.8 does not build with system-libpng anymore, further info QT-Wiki

ttuegel · 2017-03-25T13:49:57Z

I can't test on macOS myself, but this since this is just a copy of the Qt 5.7 Darwin patches, it looks good enough!

periklis · 2017-03-25T14:01:31Z

@ttuegel i am building this for x86_64-linux and x86_64-darwin on my company's hydra and use it currently myself.

LnL7 added the 6.topic: darwin Running or building packages on Darwin label Mar 20, 2017

orivej reviewed Mar 21, 2017

View reviewed changes

periklis force-pushed the topic_qt58_darwin_compatibility branch 3 times, most recently from 5bf2678 to 3181a05 Compare March 22, 2017 14:23

periklis changed the title ~~[WIP] qt58: enable darwin compatibility~~ qt58: enable darwin compatibility Mar 22, 2017

periklis force-pushed the topic_qt58_darwin_compatibility branch 4 times, most recently from c6b135f to 89fd0a8 Compare March 24, 2017 07:13

qt58: enable darwin compatibility

b97dd5f

periklis force-pushed the topic_qt58_darwin_compatibility branch from 89fd0a8 to b97dd5f Compare March 25, 2017 08:48

periklis mentioned this pull request Mar 25, 2017

Drop Qt 5.5 and Qt 5.7 from master #24299

Merged

7 tasks

ttuegel merged commit a917289 into NixOS:master Mar 25, 2017

periklis deleted the topic_qt58_darwin_compatibility branch July 25, 2018 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qt58: enable darwin compatibility #24139

qt58: enable darwin compatibility #24139

periklis commented Mar 20, 2017 •

edited

mention-bot commented Mar 20, 2017

orivej Mar 21, 2017

periklis Mar 21, 2017

vcunat Mar 21, 2017

periklis Mar 21, 2017

vcunat Mar 21, 2017

orivej Mar 21, 2017

periklis Mar 21, 2017

orivej Mar 21, 2017

periklis Mar 21, 2017

periklis Mar 21, 2017

orivej Mar 21, 2017

orivej Mar 21, 2017

orivej Mar 21, 2017

periklis Mar 21, 2017

periklis commented Mar 21, 2017

periklis commented Mar 22, 2017

ttuegel commented Mar 25, 2017

periklis commented Mar 25, 2017

qt58: enable darwin compatibility #24139

qt58: enable darwin compatibility #24139

Conversation

periklis commented Mar 20, 2017 • edited

Motivation for this change

Things done

mention-bot commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis commented Mar 21, 2017

periklis commented Mar 22, 2017

ttuegel commented Mar 25, 2017

periklis commented Mar 25, 2017

periklis commented Mar 20, 2017 •

edited