[bug#30340] [PATCH 0/6] Adopt NixOS patches for Qt5

[bug#30340] [PATCH 0/6] Adopt NixOS patches for Qt5

[Hartmut Goebel]

I reviewed the patches NixOS aplies to Qt 5.9 and 5.10. The changes of this
series hold the result. I stepped over these when trying to make KDE Plasma
work.

Most changes are about using store-paths to other packages and for dynamically
loaded libs.  To ease futuer work, I also added comments about patches we do
not need and why.


Hartmut Goebel (6):
  gnu: qtbase: Use the store paths for other packages and dynamically
    loaded libs.
  gnu: qtdeclarative: Add note about a patch NixOS has but we don't
    need.
  gnu: qtscript: Add note about a patch NixOS has but we don't need.
  gnu: qtserialport: Use the store paths for dynamically loaded libs.
  gnu: qttools: Add note about a patch NixOS has but we don't need.
  gnu: qtwebkit: Add note about a patch NixOS has but we don't need.

 gnu/packages/qt.scm | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

-- 
2.13.6

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

Adobt the NixOS patches as of 2018-01-19:

- .cmake.in and .prf files are not patches.

- src/corelib/tools/qtimezoneprivate_tz.cpp: NixOS uses $TZDIR, we use
  hardcoded path to tzdata.

- src/corelib/kernel/qcoreapplication.cpp: NixOS adds plugin paths derived
  from PATH. We do not need this, since we have native-search-path
  QT_PLUGIN_PATH.

- src/network/kernel/qdnslookup_unix.cpp,
  src/network/kernel/qhostinfo_unix.cpp: Use hardcoded path to libresolv.

- src/network/ssl/qsslcontext_openssl.cpp: NixOS changes a conditional
  compilation for Qt 5.9 (but leaves it unchanged for Qt 5.10) to fix
  compilation with libressl.  But Qt does not support libressl anway, see
  config.tests/openssl/openssl.cpp in qtbase 5.9.4.

- src/plugins/platforminputcontexts/compose/generator/qtablegenerator.cpp:
  Use hardcoded path to libx11.

- src/plugins/platforms/xcb/gl_integrations/xcb_glx/qglxintegration.cpp:
  Use hardcoded path to mess's libGL, no need for a fall-back.

- src/plugins/platforms/xcb/qxcbcursor.cpp: Use hardcoded path to Xcursor.

- src/plugins/platformthemes/gtk3/main.cpp: NixOS changes $XDG_DATA_DIRS and
  $GIO_EXTRA_MODULES in the code. We use search-path-specification for this.

- src/testlib/qtestassert.h: Unchanged for guix.

* gnu/packages/qt.scm (qtbase) Add comment. [inputs]: Add tzdata.
  [aguments]<phases>: Add 'patch-paths'.
---
 gnu/packages/qt.scm | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 8bd51ae66..606c5035a 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -35,6 +35,7 @@
   #:use-module (guix packages)
   #:use-module (guix utils)
   #:use-module (gnu packages)
+  #:use-module (gnu packages base)
   #:use-module (gnu packages bison)
   #:use-module (gnu packages compression)
   #:use-module (gnu packages cups)
@@ -430,6 +431,7 @@ developers using C++ or QML, a CSS & JavaScript like language.")
        ("postgresql" ,postgresql)
        ("pulseaudio" ,pulseaudio)
        ("sqlite" ,sqlite)
+       ("tzdata" ,tzdata)
        ("unixodbc" ,unixodbc)
        ("xcb-util" ,xcb-util)
        ("xcb-util-image" ,xcb-util-image)
@@ -540,7 +542,42 @@ developers using C++ or QML, a CSS & JavaScript like language.")
                           "qt_config.prf" "winrt/package_manifest.prf"))
                  (("\\$\\$\\[QT_HOST_DATA/get\\]") archdata)
                  (("\\$\\$\\[QT_HOST_DATA/src\\]") archdata))
-               #t))))))
+               #t)))
+         (add-after 'unpack 'patch-paths
+           ;; Use the absolute paths for dynamically loaded libs, otherwise
+           ;; the lib will be searched in the actual executable's RUNPATH,
+           ;; which may not include the requested lib.
+           (lambda* (#:key inputs #:allow-other-keys)
+             ;; tzdata
+             (substitute* "src/corelib/tools/qtimezoneprivate_tz.cpp"
+               (("\"/usr(/(share|lib)/zoneinfo/)" _ path _)
+                (string-append "\"" (assoc-ref inputs "tzdata") path)))
+             ;; libresolve. TODO: Check is this is really required
+             (let ((glibc (assoc-ref inputs ,(if (%current-target-system)
+                                                 "cross-libc" "libc"))))
+               (substitute* '("src/network/kernel/qdnslookup_unix.cpp"
+                              "src/network/kernel/qhostinfo_unix.cpp")
+                 (("^\\s*(lib.setFileName\\(QLatin1String\\(\")(resolv\"\\)\\);)" _ a b)
+                (string-append a glibc "/lib/lib" b))))
+             ;; X11/locale (compose path)
+             (substitute* "src/plugins/platforminputcontexts/compose/generator/qtablegenerator.cpp"
+               ;; Don't search in /usr/…/X11/locale, …
+               (("^\\s*m_possibleLocations.append\\(QStringLiteral\\(\"/usr/.*/X11/locale\"\\)\\);" line)
+                (string-append "// " line))
+               ;; … but use libx11's path
+               (("^\\s*(m_possibleLocations.append\\(QStringLiteral\\()X11_PREFIX \"(/.*/X11/locale\"\\)\\);)" _ a b)
+                (string-append a "\"" (assoc-ref inputs "libx11") b)))
+             ;; libGL
+             (substitute* "src/plugins/platforms/xcb/gl_integrations/xcb_glx/qglxintegration.cpp"
+               (("^\\s*(QLibrary lib\\(QLatin1String\\(\")(GL\"\\)\\);)" _ a b)
+                (string-append a (assoc-ref inputs "mesa") "/lib/lib" b)))
+             ;; libXcusor
+             (substitute* "src/plugins/platforms/xcb/qxcbcursor.cpp"
+               (("^\\s*(QLibrary xcursorLib\\(QLatin1String\\(\")(Xcursor\"\\), 1\\);)" _ a b)
+                (string-append a (assoc-ref inputs "libxcursor") "/lib/lib" b))
+               (("^\\s*(xcursorLib.setFileName\\(QLatin1String\\(\")(Xcursor\"\\)\\);)" _ a b)
+                (string-append a (assoc-ref inputs "libxcursor") "/lib/lib" b)))
+             #t)))))
     (native-search-paths
      (list (search-path-specification
             (variable "QMAKEPATH")
-- 
2.13.6

[bug#30340] [PATCH 2/6] gnu: qtdeclarative: Add note about a patch NixOS has but we don't need.

[Hartmut Goebel]

* gnu/packages/qt.scm(qtdeclarative): Add comment.
---
 gnu/packages/qt.scm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 606c5035a..9050fe113 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -773,6 +773,8 @@ xmlpatternsvalidator.")))
     (arguments
      (substitute-keyword-arguments (package-arguments qtsvg)
        ((#:tests? _ #f) #f))) ; TODO: Enable the tests
+    ;; Note: NixOS has a patch to add import paths derived from PATH. We do
+    ;; not need this, since we have native-search-path QML2_IMPORT_PATH.
     (native-inputs
      `(("perl" ,perl)
        ("pkg-config" ,pkg-config)
-- 
2.13.6

[bug#30340] [PATCH 2/6] gnu: qtdeclarative: Add note about a patch NixOS has but we don't need.

[Danny Milosavljevic]

LGTM!

[bug#30340] [PATCH 3/6] gnu: qtscript: Add note about a patch NixOS has but we don't need.

[Hartmut Goebel]

---
 gnu/packages/qt.scm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 9050fe113..45495dd6c 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -1127,6 +1127,9 @@ that helps in Qt development.")))
        ("qttools" ,qttools)))
     (inputs
      `(("qtbase" ,qtbase)))
+    ;; Note: NixOS has a patch to change a typedef in
+    ;; "src/3rdparty/javascriptcore/JavaScriptCore/wtf/Threading.h" to compile
+    ;; with glib-2.32.  We don't need this, since we are using pthreads.
     (synopsis "Qt Script module")
     (description "Qt provides support for application scripting with ECMAScript.
 The following guides and references cover aspects of programming with
-- 
2.13.6

[bug#30340] [PATCH 3/6] gnu: qtscript: Add note about a patch NixOS has but we don't need.

[Danny Milosavljevic]

LGTM!

[bug#30340] [PATCH 4/6] gnu: qtserialport: Use the store paths for dynamically loaded libs.

[Hartmut Goebel]

Adobt the NixOS patches as of 2018-01-19 for qtserialport.

* gnu/packages/qt.scm(qtserialport)[#:phases]<patch-dlopen-paths>:
  New phase.
---
 gnu/packages/qt.scm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 45495dd6c..4fc108dac 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -1002,6 +1002,18 @@ compositor libraries.")))
     (inputs
      `(("qtbase" ,qtbase)
        ("eudev" ,eudev)))
+    (arguments
+     (substitute-keyword-arguments (package-arguments qtsvg)
+       ((#:phases phases)
+        `(modify-phases ,phases
+           (add-after 'unpack 'patch-dlopen-paths
+           (lambda* (#:key inputs #:allow-other-keys)
+             (substitute* "src/serialport/qtudev_p.h"
+               ;; Use the absolute path, otherwise the lib will be searched in
+               ;; the actual executable's RUNPATH, which may not include libudev.
+               (("^\\s*(udevLibrary->setFileNameAndVersion\\(QStringLiteral\\(\")(udev\"\\),\\s*[0-9]+\\);)" _ a b)
+                (string-append a (assoc-ref inputs "eudev") "/lib/lib" b)))
+             #t))))))
     (synopsis "Qt Serial Port module")
     (description "The Qt Serial Port module provides the library for
 interacting with serial ports from within Qt.")))
-- 
2.13.6

[bug#30340] [PATCH 4/6] gnu: qtserialport: Use the store paths for dynamically loaded libs.

[Danny Milosavljevic]

On Sat,  3 Feb 2018 20:25:03 +0100
Hartmut Goebel <h.goebel@crazy-compilers.com> wrote:

> Adobt the NixOS patches as of 2018-01-19 for qtserialport.
     ^p

LGTM otherwise.

[bug#30340] [PATCH 5/6] gnu: qttools: Add note about a patch NixOS has but we don't need.

[Hartmut Goebel]

---
 gnu/packages/qt.scm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 4fc108dac..e607b8248 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -1109,6 +1109,8 @@ positioning and geolocation plugins.")))
     (arguments
      (substitute-keyword-arguments (package-arguments qtsvg)
        ((#:tests? _ #f) #f))) ; TODO: Enable the tests
+    ;; Note: NixOS has a patch to fix path to qhelpgenerator in cmake files to
+    ;; fix a build-error in KDevelop. We don't experience the build-error.
     (native-inputs
      `(("perl" ,perl)
        ("qtdeclarative" ,qtdeclarative)))
-- 
2.13.6

[bug#30340] [PATCH 5/6] gnu: qttools: Add note about a patch NixOS has but we don't need.

[Danny Milosavljevic]

LGTM!

[bug#30340] [PATCH 6/6] gnu: qtwebkit: Add note about a patch NixOS has but we don't need.

[Hartmut Goebel]

* gnu/packages/qt.scm(qtwebkit): Add comment.
---
 gnu/packages/qt.scm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index e607b8248..dc20416aa 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -1956,6 +1956,10 @@ different kinds of sliders, and much more.")
     (arguments
      `(#:phases
        (modify-phases %standard-phases
+         ;; Note: NixOS has a patch to use the absolute path for loading
+         ;; libgtk-x11-2.0.  We don't have gtk+ as an input and the respective
+         ;; files (e.g. in Source/WebCore/plugins/qt/) are not compiled at
+         ;; all.
          (add-before 'configure 'fix-qmlwebkit-plugins-rpath
            (lambda _
              (substitute* "Source/WebKit/qt/declarative/experimental/experimental.pri"
-- 
2.13.6

[bug#30340] [PATCH 6/6] gnu: qtwebkit: Add note about a patch NixOS has but we don't need.

[Danny Milosavljevic]

LGTM!

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Danny Milosavljevic]

> +             ;; libresolve. TODO: Check is this is really required
> +             (let ((glibc (assoc-ref inputs ,(if (%current-target-system)
> +                                                 "cross-libc" "libc"))))
> +               (substitute* '("src/network/kernel/qdnslookup_unix.cpp"
> +                              "src/network/kernel/qhostinfo_unix.cpp")
> +                 (("^\\s*(lib.setFileName\\(QLatin1String\\(\")(resolv\"\\)\\);)" _ a b)
> +                (string-append a glibc "/lib/lib" b))))

Any news on this?

>+             ;; libXcu[ ]sor
                         ^r


Otherwise LGTM.

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

Am 06.02.2018 um 10:00 schrieb Danny Milosavljevic:
>> +             ;; libresolve. TODO: Check is this is really required
>> +             (let ((glibc (assoc-ref inputs ,(if (%current-target-system)
>> +                                                 "cross-libc" "libc"))))
>> +               (substitute* '("src/network/kernel/qdnslookup_unix.cpp"
>> +                              "src/network/kernel/qhostinfo_unix.cpp")
>> +                 (("^\\s*(lib.setFileName\\(QLatin1String\\(\")(resolv\"\\)\\);)" _ a b)
>> +                (string-append a glibc "/lib/lib" b))))
> Any news on this?
>
I'm afraid, I don't understand this question. I don't know if "this is
really required", otherwise I would not have put the comment there.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |


[-- Attachment #2: Type: text/html, Size: 1578 bytes --]

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Danny Milosavljevic]

Hi Hartmut,

On Tue, 6 Feb 2018 12:57:32 +0100
Hartmut Goebel <h.goebel@crazy-compilers.com> wrote:

>>>[-lresolv -> glibc]

> > Any news on this?

> I'm afraid, I don't understand this question. I don't know if "this is
> really required", otherwise I would not have put the comment there.

Oh, I thought you investigated or asked someone upstream about it.

I don't know Qt interna so well - but it would be possible to ask upstream.

The part under discussion is

static bool resolveLibraryInternal()
{
    QLibrary lib;
#ifdef LIBRESOLV_SO
    lib.setFileName(QStringLiteral(LIBRESOLV_SO));
    if (!lib.load())
#endif
    {
        lib.setFileName(QLatin1String("resolv")); <-----
        if (!lib.load())
            return false;
...

so I guess it can't hurt to substitute something in the line with the arrow,
but LIBRESOLV_SO is more important, right?

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

Hi Danny,

Am 06.02.2018 um 18:54 schrieb Danny Milosavljevic:
> The part under discussion is
> static bool resolveLibraryInternal()
> {
>     QLibrary lib;
> #ifdef LIBRESOLV_SO
>     lib.setFileName(QStringLiteral(LIBRESOLV_SO));
>     if (!lib.load())
> #endif
>     {
>         lib.setFileName(QLatin1String("resolv")); <-----
>         if (!lib.load())
>             return false;
> ...
>
> so I guess it can't hurt to substitute something in the line with the arrow,
> but LIBRESOLV_SO is more important, right?


Thanks a lot for this valuable hint! I investigated this:

1) LIBRESOLV_SO seems to be not defined. The reason seems to be that
__GNU_LIBRARY__ is not defined and thus gnu/lib-names.h is not included.
(I grepped the code and build-reesults just after the build stage.)

2) Even if LIBRESOLV_SO would be defined, we would need to append the
store path. (No problem, though.)

Any idea how to solve 1)?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 3566 bytes --]

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Adobt the NixOS patches as of 2018-01-19:

I don't see any patches in this series.  FWIW I think we deviate enough
from NixOS at this point that the comments are unnecessary.

> - .cmake.in and .prf files are not patches.
>
> - src/corelib/tools/qtimezoneprivate_tz.cpp: NixOS uses $TZDIR, we use
>   hardcoded path to tzdata.

Why hardcode the path?  We set TZDIR as well in (gnu system).

[...]

> @@ -540,7 +542,42 @@ developers using C++ or QML, a CSS & JavaScript like language.")
>                            "qt_config.prf" "winrt/package_manifest.prf"))
>                   (("\\$\\$\\[QT_HOST_DATA/get\\]") archdata)
>                   (("\\$\\$\\[QT_HOST_DATA/src\\]") archdata))
> -               #t))))))
> +               #t)))
> +         (add-after 'unpack 'patch-paths
> +           ;; Use the absolute paths for dynamically loaded libs, otherwise
> +           ;; the lib will be searched in the actual executable's RUNPATH,
> +           ;; which may not include the requested lib.

Is there any reason we cannot add these libraries to RUNPATH instead?
The below approach seems somewhat fragile to me.

> +           (lambda* (#:key inputs #:allow-other-keys)
> +             ;; tzdata
> +             (substitute* "src/corelib/tools/qtimezoneprivate_tz.cpp"
> +               (("\"/usr(/(share|lib)/zoneinfo/)" _ path _)
> +                (string-append "\"" (assoc-ref inputs "tzdata") path)))
> +             ;; libresolve. TODO: Check is this is really required
> +             (let ((glibc (assoc-ref inputs ,(if (%current-target-system)
> +                                                 "cross-libc" "libc"))))
> +               (substitute* '("src/network/kernel/qdnslookup_unix.cpp"
> +                              "src/network/kernel/qhostinfo_unix.cpp")
> +                 (("^\\s*(lib.setFileName\\(QLatin1String\\(\")(resolv\"\\)\\);)" _ a b)
> +                (string-append a glibc "/lib/lib" b))))
> +             ;; X11/locale (compose path)
> +             (substitute* "src/plugins/platforminputcontexts/compose/generator/qtablegenerator.cpp"
> +               ;; Don't search in /usr/…/X11/locale, …
> +               (("^\\s*m_possibleLocations.append\\(QStringLiteral\\(\"/usr/.*/X11/locale\"\\)\\);" line)
> +                (string-append "// " line))
> +               ;; … but use libx11's path
> +               (("^\\s*(m_possibleLocations.append\\(QStringLiteral\\()X11_PREFIX \"(/.*/X11/locale\"\\)\\);)" _ a b)
> +                (string-append a "\"" (assoc-ref inputs "libx11") b)))
> +             ;; libGL
> +             (substitute* "src/plugins/platforms/xcb/gl_integrations/xcb_glx/qglxintegration.cpp"
> +               (("^\\s*(QLibrary lib\\(QLatin1String\\(\")(GL\"\\)\\);)" _ a b)
> +                (string-append a (assoc-ref inputs "mesa") "/lib/lib" b)))
> +             ;; libXcusor
> +             (substitute* "src/plugins/platforms/xcb/qxcbcursor.cpp"
> +               (("^\\s*(QLibrary xcursorLib\\(QLatin1String\\(\")(Xcursor\"\\), 1\\);)" _ a b)
> +                (string-append a (assoc-ref inputs "libxcursor") "/lib/lib" b))
> +               (("^\\s*(xcursorLib.setFileName\\(QLatin1String\\(\")(Xcursor\"\\)\\);)" _ a b)
> +                (string-append a (assoc-ref inputs "libxcursor") "/lib/lib" b)))
> +             #t)))))
>      (native-search-paths
>       (list (search-path-specification
>              (variable "QMAKEPATH")
> -- 
> 2.13.6

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Am 07.02.2018 um 17:16 schrieb Marius Bakke:
> Hartmut Goebel <h.goebel@crazy-compilers.com> writes:
>
>> Adobt the NixOS patches as of 2018-01-19:
> I don't see any patches in this series. 

I only *adopted* what NixOs does with patches, not the patches itself. I
will rework this.

>  FWIW I think we deviate enough
> from NixOS at this point that the comments are unnecessary.

Are you referring to patch (1/6)? Or do you mean patches 2, 3, 5 and 6
are unnecessary?

I do not care about the comments, but FMPOV it is important to document
somehow in the code or in the commits that all patches as of 2018-01-19
have been considered. an alternative would be to group these few commits
into a (very short) branch and documenting the fact in the merge-commit.

WDYT?

>> - src/corelib/tools/qtimezoneprivate_tz.cpp: NixOS uses $TZDIR, we use
>>   hardcoded path to tzdata.
> Why hardcode the path?  We set TZDIR as well in (gnu system).

The upstream code (qt.com) uses hard-coded path (/usr/share/zoneinfo/),
so for me it seems to be much more natural to simply change this - and
stay closer to upstream. NixOS seems to require TZDATA since some things
work differently compared to guix. E.g. nixos is deriving library search
paths from $PATH in some other patch. This is something guix does not need.

>> +         (add-after 'unpack 'patch-paths
>> +           ;; Use the absolute paths for dynamically loaded libs, otherwise
>> +           ;; the lib will be searched in the actual executable's RUNPATH,
>> +           ;; which may not include the requested lib.
> Is there any reason we cannot add these libraries to RUNPATH instead?
> The below approach seems somewhat fragile to me.

Rethinking this, this comment is wrong and I'll correct it. QLibrary
(which is used an all these cases) is documented with:

    When loading the library, QLibrary
    <http://doc.qt.io/qt-5/qlibrary.html> searches in all the
    system-specific library locations (e.g. |LD_LIBRARY_PATH| on Unix),
    unless the file name has an absolute path.

But guix does not set LD_LIBRARYPATH (e.g. in "guix environment"), thus
we need to have absolute paths for the libraries.

Does this make sense?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |


[-- Attachment #2: Type: text/html, Size: 3802 bytes --]

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Am 07.02.2018 um 17:16 schrieb Marius Bakke:
>> Hartmut Goebel <h.goebel@crazy-compilers.com> writes:
>>
>>> Adobt the NixOS patches as of 2018-01-19:
>> I don't see any patches in this series. 
>
> I only *adopted* what NixOs does with patches, not the patches itself. I
> will rework this.
>
>>  FWIW I think we deviate enough
>> from NixOS at this point that the comments are unnecessary.
>
> Are you referring to patch (1/6)? Or do you mean patches 2, 3, 5 and 6
> are unnecessary?
>
> I do not care about the comments, but FMPOV it is important to document
> somehow in the code or in the commits that all patches as of 2018-01-19
> have been considered. an alternative would be to group these few commits
> into a (very short) branch and documenting the fact in the merge-commit.
>
> WDYT?

I don't think the comments are all that useful.  They only add noise in
the commit log and the code -- since we have a working and up-to-date
Qt, it is implied that we don't need anything more from NixOS or
elsewhere.

>>> - src/corelib/tools/qtimezoneprivate_tz.cpp: NixOS uses $TZDIR, we use
>>>   hardcoded path to tzdata.
>> Why hardcode the path?  We set TZDIR as well in (gnu system).
>
> The upstream code (qt.com) uses hard-coded path (/usr/share/zoneinfo/),
> so for me it seems to be much more natural to simply change this - and
> stay closer to upstream. NixOS seems to require TZDATA since some things
> work differently compared to guix. E.g. nixos is deriving library search
> paths from $PATH in some other patch. This is something guix does not need.

For tzdata in particular, we are trying to reduce the number of
dependent packages so that we can update it directly on 'master'.  Often
a new tzdata release introduces changes with near-immediate effects, so
it's important to be able to update it fast.

Adding it to qtbase would add 282 new dependent packages, which is
unfortunate.  So I much prefer using TZDIR, even though it would be
technically better to reference it directly.

>>> +         (add-after 'unpack 'patch-paths
>>> +           ;; Use the absolute paths for dynamically loaded libs, otherwise
>>> +           ;; the lib will be searched in the actual executable's RUNPATH,
>>> +           ;; which may not include the requested lib.
>> Is there any reason we cannot add these libraries to RUNPATH instead?
>> The below approach seems somewhat fragile to me.
>
> Rethinking this, this comment is wrong and I'll correct it. QLibrary
> (which is used an all these cases) is documented with:
>
>     When loading the library, QLibrary
>     <http://doc.qt.io/qt-5/qlibrary.html> searches in all the
>     system-specific library locations (e.g. |LD_LIBRARY_PATH| on Unix),
>     unless the file name has an absolute path.
>
> But guix does not set LD_LIBRARYPATH (e.g. in "guix environment"), thus
> we need to have absolute paths for the libraries.
>
> Does this make sense?

Yes, that makes sense :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

Am 13.02.2018 um 23:48 schrieb Marius Bakke:
>> I do not care about the comments, but FMPOV it is important to document
>> somehow in the code or in the commits that all patches as of 2018-01-19
>> have been considered. an alternative would be to group these few commits
>> into a (very short) branch and documenting the fact in the merge-commit.
>>
>> WDYT?
> I don't think the comments are all that useful.  They only add noise in
> the commit log and the code -- since we have a working and up-to-date
> Qt, it is implied that we don't need anything more from NixOS or
> elsewhere.
>
@Ludo: What's you opinion on this? (See also
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30340#62)

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |


[-- Attachment #2: Type: text/html, Size: 1597 bytes --]

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 13.02.2018 um 23:48 schrieb Marius Bakke:
>>> I do not care about the comments, but FMPOV it is important to document
>>> somehow in the code or in the commits that all patches as of 2018-01-19
>>> have been considered. an alternative would be to group these few commits
>>> into a (very short) branch and documenting the fact in the merge-commit.
>>>
>>> WDYT?
>> I don't think the comments are all that useful.  They only add noise in
>> the commit log and the code -- since we have a working and up-to-date
>> Qt, it is implied that we don't need anything more from NixOS or
>> elsewhere.
>>
> @Ludo: What's you opinion on this? (See also
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30340#62)

I don’t have much to add.  It’s good to add comments for things that
aren’t obvious by looking at the ‘substitute*’ sequences, but for
obvious things it’s “noise” indeed.

At any rate, this shouldn’t hold the patches back.

Thanks,
Ludo’.

[bug#30340] Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

Am 16.02.2018 um 17:49 schrieb Ludovic Courtès:
> I don’t have much to add.  It’s good to add comments for things that
> aren’t obvious by looking at the ‘substitute*’ sequences, but for
> obvious things it’s “noise” indeed.

This is a wise, while abstract answer. :-) Unfortunately it does not
help me to decide and finish the patches. :-\

- Should I keep or drop these "Add note about a patch NixOS …" patches
(2,3,5,6)
- If dropping: Should I use a short living branch as described in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30340#62


-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Ludovic Courtès]

Hi Hartmut,

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> - Should I keep or drop these "Add note about a patch NixOS …" patches
> (2,3,5,6)

IMO yes.

> - If dropping: Should I use a short living branch as described in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30340#62

IMO no—as you know we keep the history linear whenever possible.

At the end of the day, what matters is that these patches get in.  We
shouldn’t spend so long rehashing our views on how to write comments.

Also, you have commit access because as a group we trust you to be able
to make simple decisions in line with the group conventions and
practices.  Once you’ve had initial feedback, you should feel empowered.

Thanks for working on this!

Ludo’.

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

On Sat, Feb 17, 2018 at 05:08:40PM +0100, Ludovic Courtès wrote:
> Also, you have commit access because as a group we trust you to be able
> to make simple decisions in line with the group conventions and
> practices.  Once you’ve had initial feedback, you should feel empowered.

+1

Nothing will ever be perfect :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

Am 13.02.2018 um 23:48 schrieb Marius Bakke:
> For tzdata in particular, we are trying to reduce the number of
> dependent packages so that we can update it directly on 'master'.  Often
> a new tzdata release introduces changes with near-immediate effects, so
> it's important to be able to update it fast.
>
> Adding it to qtbase would add 282 new dependent packages, which is
> unfortunate.  So I much prefer using TZDIR, even though it would be
> technically better to reference it directly.
>

IC. Should not be much of a problem to adopt the actual patch from NixOS.

But I wonder: How is ensured that timezone is installed e.g. in an `guix
environment`, if there is no dependency?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Am 13.02.2018 um 23:48 schrieb Marius Bakke:
>> For tzdata in particular, we are trying to reduce the number of
>> dependent packages so that we can update it directly on 'master'.  Often
>> a new tzdata release introduces changes with near-immediate effects, so
>> it's important to be able to update it fast.
>>
>> Adding it to qtbase would add 282 new dependent packages, which is
>> unfortunate.  So I much prefer using TZDIR, even though it would be
>> technically better to reference it directly.
>>
>
> IC. Should not be much of a problem to adopt the actual patch from NixOS.
>
> But I wonder: How is ensured that timezone is installed e.g. in an `guix
> environment`, if there is no dependency?

Good question.  I suppose we could add a search path for TZDIR so that
the variable gets set as long as you include tzdata in the environment.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Ludovic Courtès]

Hi Hartmut,

This sounds like a great improvement!

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Adobt the NixOS patches as of 2018-01-19:
>
> - .cmake.in and .prf files are not patches.
>
> - src/corelib/tools/qtimezoneprivate_tz.cpp: NixOS uses $TZDIR, we use
>   hardcoded path to tzdata.
>
> - src/corelib/kernel/qcoreapplication.cpp: NixOS adds plugin paths derived
>   from PATH. We do not need this, since we have native-search-path
>   QT_PLUGIN_PATH.
>
> - src/network/kernel/qdnslookup_unix.cpp,
>   src/network/kernel/qhostinfo_unix.cpp: Use hardcoded path to libresolv.
>
> - src/network/ssl/qsslcontext_openssl.cpp: NixOS changes a conditional
>   compilation for Qt 5.9 (but leaves it unchanged for Qt 5.10) to fix
>   compilation with libressl.  But Qt does not support libressl anway, see
>   config.tests/openssl/openssl.cpp in qtbase 5.9.4.
>
> - src/plugins/platforminputcontexts/compose/generator/qtablegenerator.cpp:
>   Use hardcoded path to libx11.
>
> - src/plugins/platforms/xcb/gl_integrations/xcb_glx/qglxintegration.cpp:
>   Use hardcoded path to mess's libGL, no need for a fall-back.
>
> - src/plugins/platforms/xcb/qxcbcursor.cpp: Use hardcoded path to Xcursor.
>
> - src/plugins/platformthemes/gtk3/main.cpp: NixOS changes $XDG_DATA_DIRS and
>   $GIO_EXTRA_MODULES in the code. We use search-path-specification for this.
>
> - src/testlib/qtestassert.h: Unchanged for guix.

I don’t understand all of this (does it describe problems or solutions?
what does it mean “files are not patches”? etc.) and I think we should
describe the problems/solutions on their own, without “NixOS does this”
comments, which isn’t really helpful IMO.

As an aside, I think explanations when they’re needed, should go in the
source, not in the commit log.

> * gnu/packages/qt.scm (qtbase) Add comment. [inputs]: Add tzdata.
>   [aguments]<phases>: Add 'patch-paths'.

[...]

> +         (add-after 'unpack 'patch-paths
> +           ;; Use the absolute paths for dynamically loaded libs, otherwise
> +           ;; the lib will be searched in the actual executable's RUNPATH,
> +           ;; which may not include the requested lib.
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             ;; tzdata
> +             (substitute* "src/corelib/tools/qtimezoneprivate_tz.cpp"
> +               (("\"/usr(/(share|lib)/zoneinfo/)" _ path _)
> +                (string-append "\"" (assoc-ref inputs "tzdata") path)))
> +             ;; libresolve. TODO: Check is this is really required

I think you can remove “TODO” here.

> +             (let ((glibc (assoc-ref inputs ,(if (%current-target-system)
> +                                                 "cross-libc" "libc"))))
> +               (substitute* '("src/network/kernel/qdnslookup_unix.cpp"
> +                              "src/network/kernel/qhostinfo_unix.cpp")
> +                 (("^\\s*(lib.setFileName\\(QLatin1String\\(\")(resolv\"\\)\\);)" _ a b)
> +                (string-append a glibc "/lib/lib" b))))
> +             ;; X11/locale (compose path)
> +             (substitute* "src/plugins/platforminputcontexts/compose/generator/qtablegenerator.cpp"
> +               ;; Don't search in /usr/…/X11/locale, …
> +               (("^\\s*m_possibleLocations.append\\(QStringLiteral\\(\"/usr/.*/X11/locale\"\\)\\);" line)
> +                (string-append "// " line))
> +               ;; … but use libx11's path
> +               (("^\\s*(m_possibleLocations.append\\(QStringLiteral\\()X11_PREFIX \"(/.*/X11/locale\"\\)\\);)" _ a b)
> +                (string-append a "\"" (assoc-ref inputs "libx11") b)))
> +             ;; libGL
> +             (substitute* "src/plugins/platforms/xcb/gl_integrations/xcb_glx/qglxintegration.cpp"
> +               (("^\\s*(QLibrary lib\\(QLatin1String\\(\")(GL\"\\)\\);)" _ a b)
> +                (string-append a (assoc-ref inputs "mesa") "/lib/lib" b)))
> +             ;; libXcusor
> +             (substitute* "src/plugins/platforms/xcb/qxcbcursor.cpp"
> +               (("^\\s*(QLibrary xcursorLib\\(QLatin1String\\(\")(Xcursor\"\\), 1\\);)" _ a b)
> +                (string-append a (assoc-ref inputs "libxcursor") "/lib/lib" b))
> +               (("^\\s*(xcursorLib.setFileName\\(QLatin1String\\(\")(Xcursor\"\\)\\);)" _ a b)
> +                (string-append a (assoc-ref inputs "libxcursor") "/lib/lib" b)))
> +             #t)))))

That makes sense to me, and actually, I don’t think it needs more
explanations.  :-)

Did you notice improvements on KDE applications?

So, LGTM!

Thanks,
Ludo’.

[bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and dynamically loaded libs.

[Hartmut Goebel]

Hi Ludo,
Am 09.02.2018 um 14:43 schrieb Ludovic Courtès:
> This sounds like a great improvement!

Thanks.
> I don’t understand all of this (does it describe problems or solutions?
> what does it mean “files are not patches”? etc.) and I think we should
> describe the problems/solutions on their own, without “NixOS does this”
> comments, which isn’t really helpful IMO.
>
> As an aside, I think explanations when they’re needed, should go in the
> source, not in the commit log.

My idea is to use the commit message to document: "We reviewed the NixOS
patches as of 2018-01-09, and this is what we decided how to handle the
respective changes." (This might still be worth rewriting.) This will
give confidence that all patches are considered to the ones working on
this next.

In contrast the code comments only document the actual code for the case
one doesn't cares about how/whether the code follows NixOS. (As you
already commented.)

 you mean patches 2, 3, 5 and 6 are unnecessary?

I'm not even sure it we'd better remove all the refernces to NixOs from
this change and only document it in commit messages. We could group
these few commits into a (very short) branch, drop patches 2, 3, 5 and
6, and documenting "considered all NixOS patched: Not needed, … etc." in
the merge-commit:

* Merge branch 'nixos patches for Qt 5.9' into master.
|\
| * gnu: qtserialport: Use the store paths for dynamically
| * gnu: qtbase: Use the store paths for
|/


The comments currently in patch 2, 3, 5, and 6 would into the Merge
commit's message.

WDYT?

> I think you can remove “TODO” here.
ACK.

> Did you notice improvements on KDE applications?

KDE Plasma feels to be more stable with these changes applied. I can't
proof it's these changes, since I have a similar series for KF5.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |