GNU bug report logs - #30340
[PATCH 0/6] Adopt NixOS patches for Qt5

Previous Next

Package: guix-patches;

Reported by: Hartmut Goebel <h.goebel <at> crazy-compilers.com>

Date: Sat, 3 Feb 2018 19:23:02 UTC

Severity: normal

Tags: patch

Done: Hartmut Goebel <h.goebel <at> crazy-compilers.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 30340 in the body.
You can then email your comments to 30340 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 03 Feb 2018 19:23:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Hartmut Goebel <h.goebel <at> crazy-compilers.com>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Sat, 03 Feb 2018 19:23:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: guix-patches <at> gnu.org
Subject: [PATCH 0/6] Adopt NixOS patches for Qt5
Date: Sat,  3 Feb 2018 20:22:12 +0100
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





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 03 Feb 2018 19:26:02 GMT) Full text and rfc822 format available.

Message #8 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340 <at> debbugs.gnu.org
Subject: [PATCH 1/6] gnu: qtbase: Use the store paths for other packages and
 dynamically loaded libs.
Date: Sat,  3 Feb 2018 20:25:00 +0100
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





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 03 Feb 2018 19:26:02 GMT) Full text and rfc822 format available.

Message #11 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340 <at> debbugs.gnu.org
Subject: [PATCH 2/6] gnu: qtdeclarative: Add note about a patch NixOS has but
 we don't need.
Date: Sat,  3 Feb 2018 20:25:01 +0100
* 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





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 03 Feb 2018 19:26:03 GMT) Full text and rfc822 format available.

Message #14 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340 <at> debbugs.gnu.org
Subject: [PATCH 3/6] gnu: qtscript: Add note about a patch NixOS has but we
 don't need.
Date: Sat,  3 Feb 2018 20:25:02 +0100
---
 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





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 03 Feb 2018 19:26:03 GMT) Full text and rfc822 format available.

Message #17 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340 <at> debbugs.gnu.org
Subject: [PATCH 4/6] gnu: qtserialport: Use the store paths for dynamically
 loaded libs.
Date: Sat,  3 Feb 2018 20:25:03 +0100
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





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 03 Feb 2018 19:26:05 GMT) Full text and rfc822 format available.

Message #20 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340 <at> debbugs.gnu.org
Subject: [PATCH 5/6] gnu: qttools: Add note about a patch NixOS has but we
 don't need.
Date: Sat,  3 Feb 2018 20:25:04 +0100
---
 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





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 03 Feb 2018 19:26:05 GMT) Full text and rfc822 format available.

Message #23 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340 <at> debbugs.gnu.org
Subject: [PATCH 6/6] gnu: qtwebkit: Add note about a patch NixOS has but we
 don't need.
Date: Sat,  3 Feb 2018 20:25:05 +0100
* 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





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 08:53:02 GMT) Full text and rfc822 format available.

Message #26 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 2/6] gnu: qtdeclarative: Add note about a
 patch NixOS has but we don't need.
Date: Tue, 6 Feb 2018 09:52:44 +0100
LGTM!




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 08:54:02 GMT) Full text and rfc822 format available.

Message #29 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 3/6] gnu: qtscript: Add note about a patch
 NixOS has but we don't need.
Date: Tue, 6 Feb 2018 09:53:14 +0100
LGTM!




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 08:55:01 GMT) Full text and rfc822 format available.

Message #32 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 5/6] gnu: qttools: Add note about a patch
 NixOS has but we don't need.
Date: Tue, 6 Feb 2018 09:54:36 +0100
LGTM!




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 08:57:02 GMT) Full text and rfc822 format available.

Message #35 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 4/6] gnu: qtserialport: Use the store paths
 for dynamically loaded libs.
Date: Tue, 6 Feb 2018 09:55:59 +0100
On Sat,  3 Feb 2018 20:25:03 +0100
Hartmut Goebel <h.goebel <at> crazy-compilers.com> wrote:

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

LGTM otherwise.




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 08:58:01 GMT) Full text and rfc822 format available.

Message #38 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 6/6] gnu: qtwebkit: Add note about a patch
 NixOS has but we don't need.
Date: Tue, 6 Feb 2018 09:57:49 +0100
LGTM!




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 09:02:01 GMT) Full text and rfc822 format available.

Message #41 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Tue, 6 Feb 2018 10:00:57 +0100
> +             ;; 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.




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 11:58:02 GMT) Full text and rfc822 format available.

Message #44 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Tue, 6 Feb 2018 12:57:32 +0100
[Message part 1 (text/plain, inline)]
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 <at> crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[Message part 2 (text/html, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 06 Feb 2018 17:55:01 GMT) Full text and rfc822 format available.

Message #47 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Tue, 6 Feb 2018 18:54:43 +0100
Hi Hartmut,

On Tue, 6 Feb 2018 12:57:32 +0100
Hartmut Goebel <h.goebel <at> 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?




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Wed, 07 Feb 2018 16:18:03 GMT) Full text and rfc822 format available.

Message #50 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>, 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Wed, 07 Feb 2018 17:16:59 +0100
[Message part 1 (text/plain, inline)]
Hartmut Goebel <h.goebel <at> 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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Thu, 08 Feb 2018 23:50:02 GMT) Full text and rfc822 format available.

Message #53 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Fri, 9 Feb 2018 00:49:18 +0100
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 <at> crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Fri, 09 Feb 2018 13:44:01 GMT) Full text and rfc822 format available.

Message #56 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Fri, 09 Feb 2018 14:43:43 +0100
Hi Hartmut,

This sounds like a great improvement!

Hartmut Goebel <h.goebel <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Mon, 12 Feb 2018 15:38:01 GMT) Full text and rfc822 format available.

Message #59 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: Marius Bakke <mbakke <at> fastmail.com>, 30340 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Mon, 12 Feb 2018 16:37:18 +0100
[Message part 1 (text/plain, inline)]
Am 07.02.2018 um 17:16 schrieb Marius Bakke:
> Hartmut Goebel <h.goebel <at> 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 <at> crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[Message part 2 (text/html, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Mon, 12 Feb 2018 16:00:02 GMT) Full text and rfc822 format available.

Message #62 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Marius Bakke <mbakke <at> fastmail.com>, 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Mon, 12 Feb 2018 16:59:16 +0100
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 <at> crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Tue, 13 Feb 2018 22:49:01 GMT) Full text and rfc822 format available.

Message #65 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>, 30340 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Tue, 13 Feb 2018 23:48:18 +0100
[Message part 1 (text/plain, inline)]
Hartmut Goebel <h.goebel <at> crazy-compilers.com> writes:

> Am 07.02.2018 um 17:16 schrieb Marius Bakke:
>> Hartmut Goebel <h.goebel <at> 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 :)
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Fri, 16 Feb 2018 16:19:02 GMT) Full text and rfc822 format available.

Message #68 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Cc: Marius Bakke <mbakke <at> fastmail.com>
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Fri, 16 Feb 2018 17:18:30 +0100
[Message part 1 (text/plain, inline)]
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 <at> crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[Message part 2 (text/html, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Fri, 16 Feb 2018 16:27:01 GMT) Full text and rfc822 format available.

Message #71 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: Marius Bakke <mbakke <at> fastmail.com>, 30340 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Fri, 16 Feb 2018 17:26:03 +0100
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 <at> crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Fri, 16 Feb 2018 16:37:02 GMT) Full text and rfc822 format available.

Message #74 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>, 30340 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Fri, 16 Feb 2018 17:36:28 +0100
[Message part 1 (text/plain, inline)]
Hartmut Goebel <h.goebel <at> 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.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Fri, 16 Feb 2018 16:50:01 GMT) Full text and rfc822 format available.

Message #77 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: Marius Bakke <mbakke <at> fastmail.com>, 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Fri, 16 Feb 2018 17:49:33 +0100
Hartmut Goebel <h.goebel <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Fri, 16 Feb 2018 18:50:02 GMT) Full text and rfc822 format available.

Message #80 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Marius Bakke <mbakke <at> fastmail.com>, 30340 <at> debbugs.gnu.org
Subject: Re: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Fri, 16 Feb 2018 19:49:54 +0100
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 <at> crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |





Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 17 Feb 2018 16:09:02 GMT) Full text and rfc822 format available.

Message #83 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: Marius Bakke <mbakke <at> fastmail.com>, 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Sat, 17 Feb 2018 17:08:40 +0100
Hi Hartmut,

Hartmut Goebel <h.goebel <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#30340; Package guix-patches. (Sat, 17 Feb 2018 20:26:02 GMT) Full text and rfc822 format available.

Message #86 received at 30340 <at> debbugs.gnu.org (full text, mbox):

From: Leo Famulari <leo <at> famulari.name>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Hartmut Goebel <h.goebel <at> crazy-compilers.com>, 30340 <at> debbugs.gnu.org
Subject: Re: [bug#30340] [PATCH 1/6] gnu: qtbase: Use the store paths for
 other packages and dynamically loaded libs.
Date: Sat, 17 Feb 2018 15:25:11 -0500
[Message part 1 (text/plain, inline)]
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 :)
[signature.asc (application/pgp-signature, inline)]

Reply sent to Hartmut Goebel <h.goebel <at> crazy-compilers.com>:
You have taken responsibility. (Sun, 18 Feb 2018 17:52:02 GMT) Full text and rfc822 format available.

Notification sent to Hartmut Goebel <h.goebel <at> crazy-compilers.com>:
bug acknowledged by developer. (Sun, 18 Feb 2018 17:52:02 GMT) Full text and rfc822 format available.

Message #91 received at 30340-close <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 30340-close <at> debbugs.gnu.org
Subject: Re: Adopt NixOS patches for Qt5
Date: Sun, 18 Feb 2018 18:51:45 +0100
I finally pushed this as bdf0c644d..042f7c263. Changes to the original
patches are:

* qtbase:
  - Don't use tzdata as input, adopt patch for using TZDIR instead
  - Update comment why store-paths are needed
* qtserialport:
  - Update comment why store-paths are needed
* drop patches 2, 3, 5 and 6 (Add not …)

-- 
Regards
Hartmut Goebel

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





bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Mon, 19 Mar 2018 11:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 7 years and 93 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.