[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
?
Your comment

This issue is archived.

To comment on this conversation send an email to 54780@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 54780
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch