[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

* gnu/packages/instrumentation.scm (lttng-ust): Fix dependencies.
[inputs]: Remove liburcu.
[propagated-inputs]: Add liburcu.

Headers of liburcu are used by headers of lttng.
---
 gnu/packages/instrumentation.scm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/instrumentation.scm b/gnu/packages/instrumentation.scm
index ab986bfcc7..45a6872268 100644
--- a/gnu/packages/instrumentation.scm
+++ b/gnu/packages/instrumentation.scm
@@ -214,7 +214,9 @@ (define-public lttng-ust
                 "1p7d94r275yvby6zqfxaswdl1q46zxbc8x5rkhnjxrp1d41byrsn"))))
     (build-system gnu-build-system)
     (inputs
-     (list liburcu numactl))
+     (list numactl))
+    (propagated-inputs
+     (list liburcu))
     (native-inputs
      (list python-3 pkg-config))
     (home-page "https://lttng.org/")
-- 
2.34.0

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

[-- Attachment #1.1: Type: text/plain, Size: 531 bytes --]

Olivier Dion via Guix-patches via schreef op do 07-04-2022 om 20:18 [-
0400]:
> Headers of liburcu are used by headers of lttng.

This can be addressed without propagation, by substitute*.  Something
like:

  (lambda* (#:key inputs #:allow-other-keys)
    (substitute* (find-files ".h")
      (("some-liburcu-header.h")
       (search-input-file inputs "include/some-liburcu-header.h"))))

Attached is some more generic and automated code I wrote a while ago.
Maybe it's good enough for lttng?

Greetings,
Maxime.

[-- Attachment #1.2: absolute-inclusions.scm --]
[-- Type: text/x-scheme, Size: 2100 bytes --]

;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
;;; GNU Guix is free software; you can redistribute it and/or modify it
;;; under the terms of the GNU General Public License as published by
;;; the Free Software Foundation; either version 3 of the License, or (at
;;; your option) any later version.
;;;
;;; GNU Guix is distributed in the hope that it will be useful, but
;;; WITHOUT ANY WARRANTY; without even the implied warranty of
;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;;; GNU General Public License for more details.
;;;
;;; You should have received a copy of the GNU General Public License
;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.

(define-module (guix build absolute-inclusions)
  #:export (absolute-inclusions patch-header-inclusions)
  #:use-module (guix build utils)
  #:use-module (ice-9 match)
  #:use-module (rnrs exceptions))

(define (absolute-inclusions files header-locations)
  (substitute* files
    ;; TODO: allow spaces before the < or include, maybe recognise #include
    ;; "foo" ...
    (("#include <(.*)>" original header-name)
     (guard (c ((search-error? c) original))
       ;; TODO: verify with libgcc & etc, maybe avoid increasing the closure size
       ;; by skipping glibc and linux headers ...
       (format #f "#include <~a>"
               (search-input-file header-locations
                                  (string-append "include/" header-name)))))))

(define* (patch-header-inclusions #:key inputs outputs #:allow-other-keys)
  "Patch inclusions in C headers in OUTPUTS to use absolute file names."
  (define header-locations (append outputs inputs))
  ;; TODO: are there also other header names in use?
  (define header-file? (file-name-predicate "\\.(h|hpp)$"))
  (for-each (match-lambda
              ((_ . output)
               (absolute-inclusions
                (find-files (string-append output "/include") header-file?)
                header-locations)))
            outputs))

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> Olivier Dion via Guix-patches via schreef op do 07-04-2022 om 20:18 [-
> 0400]:
>> Headers of liburcu are used by headers of lttng.
>
> This can be addressed without propagation, by substitute*.  Something
> like:
>
>   (lambda* (#:key inputs #:allow-other-keys)
>     (substitute* (find-files ".h")
>       (("some-liburcu-header.h")
>        (search-input-file inputs "include/some-liburcu-header.h"))))
>
> Attached is some more generic and automated code I wrote a while ago.
> Maybe it's good enough for lttng?

Is propagated-inputs not the use case for that or do I have a bad
understanding of how propagated-inputs works?

-- 
Olivier Dion
oldiob.dev

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

On Fri, 08 Apr 2022, Olivier Dion <olivier.dion@polymtl.ca> wrote:
> On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
>> Olivier Dion via Guix-patches via schreef op do 07-04-2022 om 20:18 [-
>> 0400]:
>>> Headers of liburcu are used by headers of lttng.
>>
>> This can be addressed without propagation, by substitute*.  Something
>> like:
>>
>>   (lambda* (#:key inputs #:allow-other-keys)
>>     (substitute* (find-files ".h")
>>       (("some-liburcu-header.h")
>>        (search-input-file inputs "include/some-liburcu-header.h"))))
>>
>> Attached is some more generic and automated code I wrote a while ago.
>> Maybe it's good enough for lttng?
>
> Is propagated-inputs not the use case for that or do I have a bad
> understanding of how propagated-inputs works?

To be clear about my commit.  Some headers of liburcu are required by
the application using lttng-ust.  To me, this translated into inputs
that are propagated.  Your solution would reduce the set of propagated
inputs (liburcu comes with many flavors but only one is used by
lttng-ust), by I find it ad-hoc and don't fully understand it.

-- 
Olivier Dion
oldiob.dev

[bug#54780] (C include header depropagation) Re: [bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

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

Olivier Dion schreef op vr 08-04-2022 om 10:28 [-0400]:
> To be clear about my commit.  Some headers of liburcu are required by
> the application using lttng-ust.  To me, this translated into inputs
> that are propagated.  Your solution would reduce the set of propagated
> inputs (liburcu comes with many flavors but only one is used by
> lttng-ust), by I find it ad-hoc and don't fully understand it.

The idea is to eventually make 'patch-header-inclusions' a default
phase in %standard-phases of gnu-build-system, making it non-ad-hoc.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

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

Olivier Dion schreef op vr 08-04-2022 om 10:28 [-0400]:
>  and don't fully understand it.

lttn-ust probably has some header
/gnu/store/...-lttng-unst-VERSION/include/lttng.h or the like.
It would look something like:

  [...]
  #include <liburcu.h>
  int lttng_foo(urcu_stuff *bar);
  [..]

Then the 'patch-header-inclusion' phase detects the #include
<liburcu.h>, looks for include/liburcu.h in the package inputs, and
finds /gnu/store/...-liburcu-VERSION/include/liburcu.h.  It then
replaces liburcu.h by /gnu/store/...-liburcu-VERSION/include/liburcu.h:

  [...]
  #include </gnu/store/.../include/liburcu.h>
  int lttng_foo(urcu_stuff *bar);
  [...]

Now, suppose I build an application dependning on lttng-ust.  Then the
C compiler will ‘include’ 'lttng.h' in the {CROSS_,}C_INLUDE_PATH. 
Then it sees:

  #include </gnu/store/.../include/liburcu.h>

Now, as this is an absolute /gnu/store/... file name, the compiler
knows where to find it without looking into {CROSS_,}C_INCLUDE_PATH, so
it will find the header even though it might not be in
{CROSS_,}C_INCLUDE_PATH.

It's the same system as doing some 'substitute*' to bake in the
absolute file name of some executable into the compiled application to
avoid relying on PATH, except applied to C headers instead of
executables and {CROSS_,}C_CINCLUDE_PATH instead of PATH.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> Olivier Dion schreef op vr 08-04-2022 om 10:28 [-0400]:
>>  and don't fully understand it.
>
> lttn-ust probably has some header
> /gnu/store/...-lttng-unst-VERSION/include/lttng.h or the like.
> It would look something like:
>
>   [...]
>   #include <liburcu.h>
>   int lttng_foo(urcu_stuff *bar);
>   [..]
>
> Then the 'patch-header-inclusion' phase detects the #include
> <liburcu.h>, looks for include/liburcu.h in the package inputs, and
> finds /gnu/store/...-liburcu-VERSION/include/liburcu.h.  It then
> replaces liburcu.h by /gnu/store/...-liburcu-VERSION/include/liburcu.h:
>
>   [...]
>   #include </gnu/store/.../include/liburcu.h>
>   int lttng_foo(urcu_stuff *bar);
>   [...]
>
> Now, suppose I build an application dependning on lttng-ust.  Then the
> C compiler will ‘include’ 'lttng.h' in the {CROSS_,}C_INLUDE_PATH. 
> Then it sees:
>
>   #include </gnu/store/.../include/liburcu.h>
>
> Now, as this is an absolute /gnu/store/... file name, the compiler
> knows where to find it without looking into {CROSS_,}C_INCLUDE_PATH, so
> it will find the header even though it might not be in
> {CROSS_,}C_INCLUDE_PATH.
>
> It's the same system as doing some 'substitute*' to bake in the
> absolute file name of some executable into the compiled application to
> avoid relying on PATH, except applied to C headers instead of
> executables and {CROSS_,}C_CINCLUDE_PATH instead of PATH.

Okay cool!  Thanks for the details.  I will change my patch with your
substitute.

-- 
Olivier Dion
oldiob.dev

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> Olivier Dion schreef op vr 08-04-2022 om 10:28 [-0400]:
>>  and don't fully understand it.

> Now, suppose I build an application dependning on lttng-ust.  Then the
> C compiler will ‘include’ 'lttng.h' in the {CROSS_,}C_INLUDE_PATH. 
> Then it sees:
>
>   #include </gnu/store/.../include/liburcu.h>
>
> Now, as this is an absolute /gnu/store/... file name, the compiler
> knows where to find it without looking into {CROSS_,}C_INCLUDE_PATH, so
> it will find the header even though it might not be in
> {CROSS_,}C_INCLUDE_PATH.

Here's what I have now:

--------------------
(modify-phases %standard-phases
         (add-after 'patch-source-shebangs 'patch-source-headers
           (lambda* (#:key inputs #:allow-other-keys)
             (substitute* (find-files "./include" ".h")
               (("<(urcu/(compiler|pointer|arch|system|uatomic|config|list|tls-compat|debug|ref|rculist).h)>" _ letters _)
                (format #f "<~a>"
                        (search-input-file inputs
                                           (string-append "include/" letters))))))))
--------------------

this seems to build the package correctly and also change the RCU
headers for lttng-ust.  However, liburcu also include some architecture
specific headers.  So I get the following error while compiling my
program:

--------------------
In file included from /gnu/store/5qk5mmffc1m9cla71jywn0qz03bk6yhi-profile/include/lttng/urcu/pointer.h:14,
                 from /gnu/store/5qk5mmffc1m9cla71jywn0qz03bk6yhi-profile/include/lttng/tracepoint-rcu.h:11,
                 from /gnu/store/5qk5mmffc1m9cla71jywn0qz03bk6yhi-profile/include/lttng/tracepoint.h:13,
                 from tracepoint.h:10,
                 from tracepoint.c:4:
/gnu/store/25nlsljfziysgbhhj9nhwfm4qn5h4b71-liburcu-0.13.1/include/urcu/arch.h:65:10: fatal error: urcu/arch/x86.h: No such file or directory
   65 | #include <urcu/arch/x86.h>
      |          ^~~~~~~~~~~~~~~~~
compilation terminated.
--------------------

How could this be fix?  That would require to modify the inputs no?

-- 
Olivier Dion
oldiob.dev

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

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

Olivier Dion schreef op vr 08-04-2022 om 13:17 [-0400]:
> --------------------
> (modify-phases %standard-phases
>          (add-after 'patch-source-shebangs 'patch-source-headers
>            (lambda* (#:key inputs #:allow-other-keys)
>              (substitute* (find-files "./include" ".h")
>                (("<(urcu/(compiler|pointer|arch|system|uatomic|config|list|tls-compat|debug|ref|rculist).h)>" _ letters _)
>                 (format #f "<~a>"
>                         (search-input-file inputs
>                                            (string-append "include/" letters))))))))

This is for the lttng-ust package, right?
The idea was to do this in a post-install phase of liburcu.

> --------------------
> 
> this seems to build the package correctly and also change the RCU
> headers for lttng-ust.  However, liburcu also include some architecture
> specific headers.  So I get the following error while compiling my
> program:
> 
> --------------------
> In file included from /gnu/store/5qk5mmffc1m9cla71jywn0qz03bk6yhi-profile/include/lttng/urcu/pointer.h:14,
>                  from /gnu/store/5qk5mmffc1m9cla71jywn0qz03bk6yhi-profile/include/lttng/tracepoint-rcu.h:11,
>                  from /gnu/store/5qk5mmffc1m9cla71jywn0qz03bk6yhi-profile/include/lttng/tracepoint.h:13,
>                  from tracepoint.h:10,
>                  from tracepoint.c:4:
> /gnu/store/25nlsljfziysgbhhj9nhwfm4qn5h4b71-liburcu-0.13.1/include/urcu/arch.h:65:10: fatal error: urcu/arch/x86.h: No such file or directory
>    65 | #include <urcu/arch/x86.h>
>       |          ^~~~~~~~~~~~~~~~~
> compilation terminated.
> --------------------
> 
> How could this be fix?   That would require to modify the inputs no?

I suggest moving the post-unpack substitute* phase from lttng-ust to a
post-install phase in liburcu.  That way, urcu/arch.h can be patched to
use an absolute file name.

Also, this ...

 (urcu/(compiler|pointer|arch|system|uatomic|config|list|tls compat|debug|ref|rculist).h)>

seems rather fragile and could easily break on future updates of lttng-ust.
The idea of the 'absolute-inclusions.scm' file I sent, is to automate things.
More concretely, my suggestion is to:

  * add absolute-inclusions.scm to the Guix repo, in guix/build
  * modify liburcu to:

  (package
    (name "liburcu")
    [...]
    (arguments
      (list #:imported-modules `(,@%gnu-build-system-modules (guix build absolute-inclusions))
            #:modules '((guix build gnu-build-system)
                        (guix build absolute-inclusions))
            #:phases
            #~(modify-phases %standard-phases
                (add-after 'install 'absolute-inclusions absolute-inclusions)))))

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> Olivier Dion schreef op vr 08-04-2022 om 13:17 [-0400]:
>   * add absolute-inclusions.scm to the Guix repo, in guix/build
>   * modify liburcu to:
>
>   (package
>     (name "liburcu")
>     [...]
>     (arguments
>       (list #:imported-modules `(,@%gnu-build-system-modules (guix build absolute-inclusions))
>             #:modules '((guix build gnu-build-system)
>                         (guix build absolute-inclusions))
>             #:phases
>             #~(modify-phases %standard-phases
>                 (add-after 'install 'absolute-inclusions
>   absolute-inclusions)))))

So I have a patch that incorporate your proposal (see next response).
However, I have to add the absolute-inclusions phases for both liburcu
and lttng-ust.

-- 
Olivier Dion
oldiob.dev

[bug#54780] [PATCH v2 1/2] guix: build: Add absolute-inclusions.scm.

[Olivier Dion via Guix-patches via]

* guix/build/absolute-inclusions.scm: New file.
* Makefile.am (MODULES): Add it here.
---
 Makefile.am                        |  1 +
 guix/build/absolute-inclusions.scm | 51 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100644 guix/build/absolute-inclusions.scm

diff --git a/Makefile.am b/Makefile.am
index fecce7c6f7..394df67016 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -181,6 +181,7 @@ MODULES =					\
   guix/diagnostics.scm				\
   guix/ui.scm					\
   guix/status.scm				\
+  guix/build/absolute-inclusions.scm		\
   guix/build/android-ndk-build-system.scm	\
   guix/build/ant-build-system.scm		\
   guix/build/download.scm			\
diff --git a/guix/build/absolute-inclusions.scm b/guix/build/absolute-inclusions.scm
new file mode 100644
index 0000000000..06b4cd7beb
--- /dev/null
+++ b/guix/build/absolute-inclusions.scm
@@ -0,0 +1,51 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2022 Olivier Dion <olivier.dion@polymtl.ca>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (guix build absolute-inclusions)
+  #:use-module (guix build utils)
+  #:use-module (ice-9 match)
+  #:use-module (rnrs exceptions)
+  #:export (absolute-inclusions patch-header-inclusions))
+
+(define (absolute-inclusions files header-locations)
+  (substitute* files
+    (("^[ \t]*#[ \t]*include[ \t]*<(.*)>[ \t]*" original header-name)
+     (guard (c ((search-error? c) original))
+       ;; TODO: verify with libgcc & etc, maybe avoid increasing the
+       ;; closure size by skipping glibc and linux headers ...
+       (format #f "#include <~a>"
+               (search-input-file header-locations
+                (string-append "include/" header-name)))))))
+
+(define header-file?
+  ;; See gcc(1) for the list of suffixes.
+  (let ((suffixes (list "h" "hh" "H" "hp" "hxx" "hpp" "HPP" "h++" "tcc")))
+    (file-name-predicate
+     (format #f "\\.(~a)$" (string-join suffixes "|" 'infix)))))
+
+(define* (patch-header-inclusions #:key inputs outputs #:allow-other-keys)
+  "Patch inclusions in C headers in OUTPUTS to use absolute file names."
+  (define header-locations (append outputs inputs))
+  (for-each (match-lambda
+              ((_ . output)
+               (absolute-inclusions
+                (find-files (string-append output "/include")
+                            header-file?)
+                header-locations)))
+            outputs))
-- 
2.34.0

[bug#54780] [PATCH v3 1/2] guix: build: Add absolute-inclusions.scm.

[Olivier Dion via Guix-patches via]

* guix/build/absolute-inclusions.scm: New file.
* Makefile.am (MODULES): Add it here.
---
 Makefile.am                        |  1 +
 guix/build/absolute-inclusions.scm | 59 ++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 guix/build/absolute-inclusions.scm

diff --git a/Makefile.am b/Makefile.am
index fecce7c6f7..394df67016 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -181,6 +181,7 @@ MODULES =					\
   guix/diagnostics.scm				\
   guix/ui.scm					\
   guix/status.scm				\
+  guix/build/absolute-inclusions.scm		\
   guix/build/android-ndk-build-system.scm	\
   guix/build/ant-build-system.scm		\
   guix/build/download.scm			\
diff --git a/guix/build/absolute-inclusions.scm b/guix/build/absolute-inclusions.scm
new file mode 100644
index 0000000000..34aecf198a
--- /dev/null
+++ b/guix/build/absolute-inclusions.scm
@@ -0,0 +1,59 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2022 Olivier Dion <olivier.dion@polymtl.ca>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (guix build absolute-inclusions)
+  #:use-module (guix build utils)
+  #:use-module (ice-9 match)
+  #:use-module (rnrs exceptions)
+  #:export (absolute-inclusions patch-header-inclusions))
+
+(define (absolute-inclusions files header-locations)
+  (substitute* files
+    (("^[ \t]*#[ \t]*include[ \t]*<(.*)>[ \t]*" original header-name)
+     (guard (c ((search-error? c) original))
+       (format #f "#include <~a>"
+               (search-input-file header-locations
+                (string-append "include/" header-name)))))))
+
+(define header-file?
+  ;; See gcc(1) for the list of suffixes.
+  (let ((suffixes (list "h" "hh" "H" "hp" "hxx" "hpp" "HPP" "h++" "tcc")))
+    (file-name-predicate
+     (format #f "\\.(~a)$" (string-join suffixes "|" 'infix)))))
+
+(define (excluded-input? input)
+  ;; Exclude glibc to avoid increasing the closure size when a
+  ;; static library + binary is build.  Likewise, exclude
+  ;; linux-libre-headers.
+  (match input
+    ((_ . store-item)
+     (or (file-exists? (string-append store-item "/include/stdlib.h"))
+         (directory-exists? (string-append store-item "/include/linux"))))))
+
+(define* (patch-header-inclusions #:key inputs outputs #:allow-other-keys)
+  "Patch inclusions in C headers in OUTPUTS to use absolute file names."
+  (define header-locations
+    (filter (negate excluded-input?) (append outputs inputs)))
+  (for-each (match-lambda
+              ((_ . output)
+               (absolute-inclusions
+                (find-files (string-append output "/include")
+                            header-file?)
+                header-locations)))
+            outputs))
-- 
2.35.1

[bug#54780] [PATCH v3 2/2] gnu: packages: Use absolute headers inclusion.

[Olivier Dion via Guix-patches via]

* gnu/packages/instrumentation.scm (lttng-ust):
[phases]: Add absolute-inclusions.

* gnu/packages/datastructures.scm (liburcu):
[phases]: Add absolute-inclusions.
---
 gnu/packages/datastructures.scm  | 11 +++++++++++
 gnu/packages/instrumentation.scm | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/gnu/packages/datastructures.scm b/gnu/packages/datastructures.scm
index f247231ecf..042fc8114e 100644
--- a/gnu/packages/datastructures.scm
+++ b/gnu/packages/datastructures.scm
@@ -26,6 +26,7 @@ (define-module (gnu packages datastructures)
   #:use-module (gnu packages autotools)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages perl)
+  #:use-module (guix gexp)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix packages)
   #:use-module (guix download)
@@ -146,6 +147,16 @@ (define-public liburcu
                (base32
                 "10rh6v9j13622cjlzx31cfpghjy0kqkvn6pb42whwwcg5cyz64rj"))))
     (build-system gnu-build-system)
+    (arguments
+      (list #:imported-modules
+            `(,@%gnu-build-system-modules (guix build absolute-inclusions))
+            #:modules '((guix build gnu-build-system)
+                        (guix build absolute-inclusions)
+                        (guix build utils))
+            #:phases
+            #~(modify-phases %standard-phases
+                (add-after 'install 'absolute-inclusions
+                  patch-header-inclusions))))
     (native-inputs
      (list perl))                 ; for tests
     (home-page "https://liburcu.org/")
diff --git a/gnu/packages/instrumentation.scm b/gnu/packages/instrumentation.scm
index ab986bfcc7..672189a068 100644
--- a/gnu/packages/instrumentation.scm
+++ b/gnu/packages/instrumentation.scm
@@ -215,6 +215,17 @@ (define-public lttng-ust
     (build-system gnu-build-system)
     (inputs
      (list liburcu numactl))
+    (arguments
+     (list
+      #:imported-modules
+            `(,@%gnu-build-system-modules (guix build absolute-inclusions))
+            #:modules '((guix build gnu-build-system)
+                        (guix build absolute-inclusions)
+                        (guix build utils))
+            #:phases
+            #~(modify-phases %standard-phases
+                (add-after 'install 'absolute-inclusions
+                  patch-header-inclusions))))
     (native-inputs
      (list python-3 pkg-config))
     (home-page "https://lttng.org/")
-- 
2.35.1

[bug#54780] [PATCH v2 2/2] gnu: packages: Use absolute headers inclusion.

[Olivier Dion via Guix-patches via]

* gnu/packages/instrumentation.scm (lttng-ust):
[phases]: Add absolute-inclusions.

* gnu/packages/datastructures.scm (liburcu):
[phases]: Add absolute-inclusions.
---
 gnu/packages/datastructures.scm  | 11 +++++++++++
 gnu/packages/instrumentation.scm | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/gnu/packages/datastructures.scm b/gnu/packages/datastructures.scm
index f247231ecf..042fc8114e 100644
--- a/gnu/packages/datastructures.scm
+++ b/gnu/packages/datastructures.scm
@@ -26,6 +26,7 @@ (define-module (gnu packages datastructures)
   #:use-module (gnu packages autotools)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages perl)
+  #:use-module (guix gexp)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix packages)
   #:use-module (guix download)
@@ -146,6 +147,16 @@ (define-public liburcu
                (base32
                 "10rh6v9j13622cjlzx31cfpghjy0kqkvn6pb42whwwcg5cyz64rj"))))
     (build-system gnu-build-system)
+    (arguments
+      (list #:imported-modules
+            `(,@%gnu-build-system-modules (guix build absolute-inclusions))
+            #:modules '((guix build gnu-build-system)
+                        (guix build absolute-inclusions)
+                        (guix build utils))
+            #:phases
+            #~(modify-phases %standard-phases
+                (add-after 'install 'absolute-inclusions
+                  patch-header-inclusions))))
     (native-inputs
      (list perl))                 ; for tests
     (home-page "https://liburcu.org/")
diff --git a/gnu/packages/instrumentation.scm b/gnu/packages/instrumentation.scm
index ab986bfcc7..672189a068 100644
--- a/gnu/packages/instrumentation.scm
+++ b/gnu/packages/instrumentation.scm
@@ -215,6 +215,17 @@ (define-public lttng-ust
     (build-system gnu-build-system)
     (inputs
      (list liburcu numactl))
+    (arguments
+     (list
+      #:imported-modules
+            `(,@%gnu-build-system-modules (guix build absolute-inclusions))
+            #:modules '((guix build gnu-build-system)
+                        (guix build absolute-inclusions)
+                        (guix build utils))
+            #:phases
+            #~(modify-phases %standard-phases
+                (add-after 'install 'absolute-inclusions
+                  patch-header-inclusions))))
     (native-inputs
      (list python-3 pkg-config))
     (home-page "https://lttng.org/")
-- 
2.34.0

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

[-- Attachment #1.1: Type: text/plain, Size: 1421 bytes --]

user guix
usertags 54780 + reviewed-looks-good
usertags 49672 + reviewed-looks-good
thanks

Olivier Dion schreef op vr 08-04-2022 om 18:56 [-0400]:
> On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> > Olivier Dion schreef op vr 08-04-2022 om 13:17 [-0400]:
> >    * add absolute-inclusions.scm to the Guix repo, in guix/build
> >    * modify liburcu to:
> > 
> >    (package
> >      (name "liburcu")
> >      [...]
> >      (arguments
> >        (list #:imported-modules `(,@%gnu-build-system-modules (guix build absolute-inclusions))
> >              #:modules '((guix build gnu-build-system)
> >                          (guix build absolute-inclusions))
> >              #:phases
> >              #~(modify-phases %standard-phases
> >                  (add-after 'install 'absolute-inclusions
> >    absolute-inclusions)))))
> 
> So I have a patch that incorporate your proposal (see next response).
> However, I have to add the absolute-inclusions phases for both liburcu
> and lttng-ust.

Right, lttng-ust can be used by other packages.
There were some unaddressed TODOs in absolute-inclusions.scm, the
attached absolute-icnlusion.scm addresses them.  But aside from that,
it looks good to me.  FWIW, I can confirm that lttng-tools builds.

Greetings,
Maxime.

[-- Attachment #1.2: absolute-inclusions.scm --]
[-- Type: text/x-scheme, Size: 2499 bytes --]

;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2022 Olivier Dion <olivier.dion@polymtl.ca>
;;;
;;; This file is part of GNU Guix.
;;;
;;; GNU Guix is free software; you can redistribute it and/or modify it
;;; under the terms of the GNU General Public License as published by
;;; the Free Software Foundation; either version 3 of the License, or (at
;;; your option) any later version.
;;;
;;; GNU Guix is distributed in the hope that it will be useful, but
;;; WITHOUT ANY WARRANTY; without even the implied warranty of
;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;;; GNU General Public License for more details.
;;;
;;; You should have received a copy of the GNU General Public License
;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.

(define-module (guix build absolute-inclusions)
  #:use-module (guix build utils)
  #:use-module (ice-9 match)
  #:use-module (rnrs exceptions)
  #:export (absolute-inclusions patch-header-inclusions))

(define (excluded-input? input)
  ;; Exclude glibc to avoid increasing the closure size when a
  ;; static library + binary is build.  Likewise, exclude
  ;; linux-libre-headers.
  (match input
    ((_ . store-item)
     (or (file-exists? (string-append store-item "/include/stdlib.h"))
         (directory-exists? (string-append store-item "/include/linux"))))))

(define (absolute-inclusions files header-locations)
  (substitute* files
    (("^[ \t]*#[ \t]*include[ \t]*<(.*)>[ \t]*" original header-name)
     (guard (c ((search-error? c) original))
       (format #f "#include <~a>"
               (pk 'foo (search-input-file header-locations
                (string-append "include/" header-name))))))))

(define header-file?
  ;; See gcc(1) for the list of suffixes.
  (let ((suffixes (list "h" "hh" "H" "hp" "hxx" "hpp" "HPP" "h++" "tcc")))
    (file-name-predicate
     (format #f "\\.(~a)$" (string-join suffixes "|" 'infix)))))

(define* (patch-header-inclusions #:key inputs outputs #:allow-other-keys)
  "Patch inclusions in C headers in OUTPUTS to use absolute file names."
  (define header-locations
    (filter (negate excluded-input?) (append outputs inputs)))
  (for-each (match-lambda
              ((_ . output)
               (absolute-inclusions
                (find-files (string-append output "/include")
                            header-file?)
                header-locations)))
            outputs))

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

[-- Attachment #1.1: Type: text/plain, Size: 287 bytes --]

Maxime Devos schreef op za 09-04-2022 om 11:11 [+0200]:
> Right, lttng-ust can be used by other packages.
> There were some unaddressed TODOs in absolute-inclusions.scm, the
> attached absolute-icnlusion.scm addresses them.  But aside from that,

Oops I forgot to remove the 'pk'

[-- Attachment #1.2: Type: text/x-scheme, Size: 2489 bytes --]

;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2022 Olivier Dion <olivier.dion@polymtl.ca>
;;;
;;; This file is part of GNU Guix.
;;;
;;; GNU Guix is free software; you can redistribute it and/or modify it
;;; under the terms of the GNU General Public License as published by
;;; the Free Software Foundation; either version 3 of the License, or (at
;;; your option) any later version.
;;;
;;; GNU Guix is distributed in the hope that it will be useful, but
;;; WITHOUT ANY WARRANTY; without even the implied warranty of
;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;;; GNU General Public License for more details.
;;;
;;; You should have received a copy of the GNU General Public License
;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.

(define-module (guix build absolute-inclusions)
  #:use-module (guix build utils)
  #:use-module (ice-9 match)
  #:use-module (rnrs exceptions)
  #:export (absolute-inclusions patch-header-inclusions))

(define (excluded-input? input)
  ;; Exclude glibc to avoid increasing the closure size when a
  ;; static library + binary is build.  Likewise, exclude
  ;; linux-libre-headers.
  (match input
    ((_ . store-item)
     (or (file-exists? (string-append store-item "/include/stdlib.h"))
         (directory-exists? (string-append store-item "/include/linux"))))))

(define (absolute-inclusions files header-locations)
  (substitute* files
    (("^[ \t]*#[ \t]*include[ \t]*<(.*)>[ \t]*" original header-name)
     (guard (c ((search-error? c) original))
       (format #f "#include <~a>"
               (search-input-file header-locations
                (string-append "include/" header-name)))))))

(define header-file?
  ;; See gcc(1) for the list of suffixes.
  (let ((suffixes (list "h" "hh" "H" "hp" "hxx" "hpp" "HPP" "h++" "tcc")))
    (file-name-predicate
     (format #f "\\.(~a)$" (string-join suffixes "|" 'infix)))))

(define* (patch-header-inclusions #:key inputs outputs #:allow-other-keys)
  "Patch inclusions in C headers in OUTPUTS to use absolute file names."
  (define header-locations
    (filter (negate excluded-input?) (append outputs inputs)))
  (for-each (match-lambda
              ((_ . output)
               (absolute-inclusions
                (find-files (string-append output "/include")
                            header-file?)
                header-locations)))
            outputs))

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

On Sat, 09 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> Maxime Devos schreef op za 09-04-2022 om 11:11 [+0200]:
> (define (excluded-input? input)
>          (directory-exists? (string-append store-item
>          "/include/linux"))))))

I'm not sure if that's okay.  What if the package require kernel's
headers?  Would this works if testing a custom kernel?

I assume that for glibc that's okay because the toolchain was built with
'--with-native-system-header-dir=DIRNAME' or something like that.

-- 
Olivier Dion
oldiob.dev

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

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

Olivier Dion schreef op za 09-04-2022 om 10:40 [-0400]:
> On Sat, 09 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> > Maxime Devos schreef op za 09-04-2022 om 11:11 [+0200]:
> > (define (excluded-input? input)
> >           (directory-exists? (string-append store-item
> >           "/include/linux"))))))
> [...]
> I assume that for glibc that's okay because the toolchain was built with
> '--with-native-system-header-dir=DIRNAME' or something like that.

Some remarks:

  * Let 'foo' be a program depending on the C library 'bar' whose
    headers include some headers from 'linux-libre-headers' and
    'glibc'.

  * Almost every C program includes some headers of glibc anyway,
    so to compile the C program 'foo', you would need to have a glibc
    in the environment anyway, so absoluting the references to glibc
    headers doesn't bring much here.

  * Even then, as you say, the toolchain is compiled with something
    like that.

> I'm not sure if that's okay.  What if the package require kernel's
> headers?  Would this works if testing a custom kernel?

  * Looking at the existence of $GUIX_ENVIRONMENT/include/linux when
    doing "guix shell -D hello --pure", it looks like (a slightly
    old version of) linux-libre-headers is included by default in the
    build environment, so the package will have some kernel headers
    automatically.

  * Suppose that 'linux-libre-headers' was not excluded.
    Suppose that 'bar' depends on 'linux-libre-headers', and hence some
    inclusions were absolutised.  Now suppose that 'foo' requires a
    _newer_ linux-libre-headers, say linux-libre-headers@5.15 to
    utilise some fancy new thing in Linux.

    Suppose the source code is something like:

    foo.c:
    #include <bar.h>
    #include <linux/stuff.h> --> absolutised to </...new-linux.../linux/stuff.h>

    bar.h:
    [standard include guard]
    #include <linux/stuff.h> ---> absolutised to </...old-linux.../linux/stuff.h>
    [standard include guard]

    linux/stuff.h
    [standard include guard]
    [stuff depending on the linux-libre-headers version]
    [standard include guard]

    Then, what would happen when 'foo.c', is that at first,
    the C compiler loads <bar.h>.  It sees that old-linux/stuff.h
    and loads it, including the include guard.  The next thing it
    sees is #include <linux/stuff.h>.  But then the compiler
    (mis)remembers, due to the include guard, that it included this
    header already, so it will not include the _new_ <linux/stuff.h>

    As such, foo.c would end up with the _old_ <linux/stuff.h>, even though
    it needed the new stuff (new structs or such).  By not absolutising the
    <linux/...>, the compiler will just look for <linux/...> in C_INCLUDE_PATH,
    and find the linux-libre-headers from lttng-ust's inputs.

    So I'm a bit hesitant to including linux-libre-headers.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

On Sat, 09 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
> Olivier Dion schreef op za 09-04-2022 om 10:40 [-0400]:
>> On Sat, 09 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
>     As such, foo.c would end up with the _old_ <linux/stuff.h>, even though
>     it needed the new stuff (new structs or such).  By not absolutising the
>     <linux/...>, the compiler will just look for <linux/...> in C_INCLUDE_PATH,
>     and find the linux-libre-headers from lttng-ust's inputs.
>
>     So I'm a bit hesitant to including linux-libre-headers.

Following your example, I now think it's best to not include the linux
headers in that case.  Time will tell if there's some very specific case
where the opposite is true.  I'll post a v3 with your proposed changes.

-- 
Olivier Dion
oldiob.dev

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Maxime Devos]

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

Olivier Dion schreef op vr 08-04-2022 om 10:23 [-0400]:
> > 0400]:
> > > Headers of liburcu are used by headers of lttng.
> > 
> > This can be addressed without propagation, by substitute*. 
> > Something
> > like:
> > 
> >    (lambda* (#:key inputs #:allow-other-keys)
> >      (substitute* (find-files ".h")
> >        (("some-liburcu-header.h")
> >         (search-input-file inputs "include/some-liburcu-
> > header.h"))))
> > 
> > Attached is some more generic and automated code I wrote a
> >  while ago. Maybe it's good enough for lttng?
> 
> Is propagated-inputs not the use case for that or do I have a bad
> understanding of how propagated-inputs works?

Propagation is the standard work-around if not better alternatives are
known.  But it has some downsides:

  * if liburcu contained a binary 'bin/urcu', then if you install
    lttng-ust, you would also get 'bin/urcu' in the profile even though
    you did not ask for it.

  * propagation is a source of slowness.

    See, e.g., <https://issues.guix.gnu.org/41702>.

  * It can also make updating individual packages (with "guix pull &&
    guix package -u this-package") more difficult since it might be
    necessary to update multiple packages at the same time to avoid
    propagation conflicts.

  * (not applicable to this case, given that the lttng-ust library
    (probably) refers to the liburcu library): "guix gc --references"
    does not known about these kind of ‘hidden’ references.

As such, when feasible, propagation is avoided.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.

[Olivier Dion via Guix-patches via]

Could we have one of the patches merge?  Either the first version or the
last.  lttng-ust is currently broken and setting aside the patch of
absolute inclusion of header files, it would be nice to have it work
until then.

Redards,
old

-- 
Olivier Dion
oldiob.dev

bug#54780: [PATCH] gnu: lttng-ust: Fix dependencies.

[Ludovic Courtès]

Hi Olivier,

Olivier Dion <olivier.dion@polymtl.ca> skribis:

> Could we have one of the patches merge?  Either the first version or the
> last.  lttng-ust is currently broken and setting aside the patch of
> absolute inclusion of header files, it would be nice to have it work
> until then.

I finally applied the first version.

Thanks and apologies for the delay!

Ludo’.