[PATCH] gnu: lttng-ust: Fix dependencies.

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxime Devos
  • Olivier Dion
Owner
unassigned
Submitted by
Olivier Dion
Severity
normal
O
O
Olivier Dion wrote on 8 Apr 2022 02:18
(address . guix-patches@gnu.org)(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
3afb4ed9e9446906504f781dcf047160491ddfff.1649377087.git.olivier.dion@polymtl.ca
* 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(-)

Toggle diff (17 lines)
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
M
M
Maxime Devos wrote on 8 Apr 2022 16:04
c577e000949d06a83288f2eed04825a90ff696af.camel@telenet.be
Olivier Dion via Guix-patches via schreef op do 07-04-2022 om 20:18 [-
0400]:
Toggle quote (2 lines)
> 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.
;;; 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))
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlBA4BccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7sOBAP9JEf/ZmAggg/YQWSEodqSTXrwg
KwQkSoAVveaHr6dzVAEAl3dcykdqkgiCTDrCJr9v0mVeP/DMyh+O7i9QUU3JWAE=
=Ij/C
-----END PGP SIGNATURE-----


O
O
Olivier Dion wrote on 8 Apr 2022 16:23
871qy7wt52.fsf@laura
On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
Toggle quote (15 lines)
> 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
O
O
Olivier Dion wrote on 8 Apr 2022 16:28
87v8vjvecz.fsf@laura
On Fri, 08 Apr 2022, Olivier Dion <olivier.dion@polymtl.ca> wrote:
Toggle quote (19 lines)
> 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
M
M
Maxime Devos wrote on 8 Apr 2022 17:23
6d081788eadef2a6a17e4fbf385ee1c08c389385.camel@telenet.be
Olivier Dion schreef op vr 08-04-2022 om 10:23 [-0400]:
Toggle quote (19 lines)
> > 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.


* 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlBThRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7tcDAQCP5WtBwYLEl4AYBdEqqhHcGLjh
yLkHf08cdGh5rDtEJwD9FeEXa076uuqpYTABb754+E+r/y8M18BFCZGomDR9wQI=
=RauP
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 8 Apr 2022 17:32
3e664c3fdc041bb69cdff0f03259e5694eced547.camel@telenet.be
Olivier Dion schreef op vr 08-04-2022 om 10:28 [-0400]:
Toggle quote (6 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlBVfxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7rgeAP44wdfnsZVgBOTenh6X5wXD0HX3
AKQ/W2bGeiAIRM6hFgD/RwwT05i7laoQfZj+JdAP7tLA4aRZF1fMGKDQMvfxuAE=
=xhBM
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 8 Apr 2022 17:39
Re: [bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.
7a376302aab455dae361205a491a4e5fd5bd5922.camel@telenet.be
Olivier Dion schreef op vr 08-04-2022 om 10:28 [-0400]:
Toggle quote (2 lines)
>  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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlBXSxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7oZ8AQCZSXtYO8e0gbzzhV+ET0hbWAaD
edv+OginZvN/G9SsrgEA3QUBSKCX1aCEMG4mRV15RoTANP5HV7wah4hRRr0/4AE=
=H2n+
-----END PGP SIGNATURE-----


O
O
Olivier Dion wrote on 8 Apr 2022 17:59
87sfqnva51.fsf@laura
On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
Toggle quote (38 lines)
> 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
O
O
Olivier Dion wrote on 8 Apr 2022 19:17
87o81bv6iz.fsf@laura
On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
Toggle quote (14 lines)
> 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
M
M
Maxime Devos wrote on 8 Apr 2022 21:41
77e920c46bbecdbc2164151b0b194ef86bb54357.camel@telenet.be
Olivier Dion schreef op vr 08-04-2022 om 13:17 [-0400]:
Toggle quote (10 lines)
> --------------------
> (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.

Toggle quote (21 lines)
> --------------------
>
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlCP7hccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7hqmAP9HpfhoHCwbrOVqOoLh+1LmAeeF
GQ9SiByJWXkBc2BhQAEAgvsrNUW7m4S5uzI4Fau+NsnLuUoSxB0Z+tAxp8oyXw4=
=Nyic
-----END PGP SIGNATURE-----


O
O
Olivier Dion wrote on 9 Apr 2022 00:56
87lewfuqtk.fsf@laura
On Fri, 08 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
Toggle quote (16 lines)
> 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
O
O
Olivier Dion wrote on 9 Apr 2022 01:15
[PATCH v2 1/2] guix: build: Add absolute-inclusions.scm.
4b19827ee0d266ecd80dc7c5d776f3ffd764a575.1649459701.git.olivier.dion@polymtl.ca
* 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

Toggle diff (71 lines)
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
O
O
Olivier Dion wrote on 9 Apr 2022 01:15
[PATCH v2 2/2] gnu: packages: Use absolute headers inclusion.
b0ec469c45de3e1919fa5571867254c547d295db.1649459701.git.olivier.dion@polymtl.ca
* 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(+)

Toggle diff (53 lines)
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
M
M
Maxime Devos wrote on 9 Apr 2022 11:11
Re: [bug#54780] [PATCH] gnu: lttng-ust: Fix dependencies.
1f20e8a21cd13b25726187ffbf665ecbfb26177e.camel@telenet.be
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]:
Toggle quote (21 lines)
> 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.
;;; 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))
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlFNuRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7kH/AP49QhuMBxyf0OEqkUYS4Y+yEY9I
LpXXlWnAaENB+01exwEAtzZaramf6cWpwAL9zJgSiIvjEufQpdOqPosrGtXjwA4=
=liXG
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 9 Apr 2022 11:13
ae5cd023f2d766f4e5913be94387a929891795ce.camel@telenet.be
Maxime Devos schreef op za 09-04-2022 om 11:11 [+0200]:
Toggle quote (4 lines)
> 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'
;;; 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))
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlFOIBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7ttMAP9JLy1vTeF3cRMY54dOVWQdUfCt
1S4HCsETBFe6VgS1rQD/TBnMFuYZ2zvJiQsbHZuzmw74f4P0woZ7dMgiWu6CKwk=
=HM07
-----END PGP SIGNATURE-----


O
O
Olivier Dion wrote on 9 Apr 2022 16:40
87fsmmuxq5.fsf@laura
On Sat, 09 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
Toggle quote (5 lines)
> 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
M
M
Maxime Devos wrote on 9 Apr 2022 18:01
e89528f27447ac16f3add164417ba9280ad58ebf.camel@telenet.be
Olivier Dion schreef op za 09-04-2022 om 10:40 [-0400]:
Toggle quote (9 lines)
> 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.

Toggle quote (3 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYlGt5hccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7vOZAQDZEQJ1wURMRNvCNjZOKjLC+PP+
RAIydYbxteulItTHoQEAnj8JCDrKpEGvWrswtS7zAKzHmavcDziiT6hdy24LZwo=
=dZ7g
-----END PGP SIGNATURE-----


O
O
Olivier Dion wrote on 9 Apr 2022 18:34
87czhqusey.fsf@laura
On Sat, 09 Apr 2022, Maxime Devos <maximedevos@telenet.be> wrote:
Toggle quote (9 lines)
> 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
O
O
Olivier Dion wrote on 21 Apr 2022 18:46
[PATCH v3 2/2] gnu: packages: Use absolute headers inclusion.
2840658be9d631e3bda8df14c1b70d0766bfe61e.1650559513.git.olivier.dion@polymtl.ca
* 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(+)

Toggle diff (53 lines)
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
O
O
Olivier Dion wrote on 21 Apr 2022 18:46
[PATCH v3 1/2] guix: build: Add absolute-inclusions.scm.
f8fd0d5178c557c999299907c09f71e08ec3402a.1650559513.git.olivier.dion@polymtl.ca
* 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

Toggle diff (79 lines)
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
O
O
Olivier Dion wrote on 17 May 2022 22:38
Re: [PATCH] gnu: lttng-ust: Fix dependencies.
(address . guix-patches@gnu.org)(name . Maxime Devos)(address . maximedevos@telenet.be)
87bkvvj3nd.fsf@laura
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
L
L
Ludovic Courtès wrote on 14 Jun 2022 23:26
Re: bug#54780: [PATCH] gnu: lttng-ust: Fix dependencies.
(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
87bkuvvsv5.fsf_-_@gnu.org
Hi Olivier,

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

Toggle quote (5 lines)
> 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’.
Closed
?