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

* 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(-)

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.

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?

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 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.

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.

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.

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.

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 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.

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.

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.

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'

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 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.

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.

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

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’.