retis-org / retis

Tracing packets in the Linux networking stack & friends
https://retis.readthedocs.io/en/stable/
100 stars 14 forks source link

build.rs: do not always rebuild on non debian based systems. #79

Closed vlrpl closed 1 year ago

vlrpl commented 1 year ago

on Fedora this could stretch build time:

$ touch src/core/events/events.rs && /bin/time -p cargo build
   Compiling packet-tracer v0.1.0
    Finished dev [unoptimized + debuginfo] target(s) in 8.96s
real 9.17
user 5.99
sys 3.02

with this applied:

$ touch src/core/events/events.rs && /bin/time -p cargo build
   Compiling packet-tracer v0.1.0
    Finished dev [unoptimized + debuginfo] target(s) in 2.28s
real 2.31
user 1.85
sys 0.44
atenart commented 1 year ago

If I understand this correctly, this is a fixup for the /usr/include/x86_64-linux-gnu header path, which is used to look for errno.h.

If the path doesn't exist (e.g. on Fedora), then do we use the right errno.h file?

vlrpl commented 1 year ago

If I understand this correctly, this is a fixup for the /usr/include/x86_64-linux-gnu header path, which is used to look for errno.h.

If the path doesn't exist (e.g. on Fedora), then do we use the right errno.h file?

Yes, the problem is that directory. AFAICT, the problem is that for debian system that include path is needed as <asm/errno.h> fails to be included otherwise:

        stderr=
         In file included from src/core/probe/user/bpf/usdt.bpf.c:3:
        In file included from /tmp/.tmpOVQRLT/bpf/src/bpf/usdt.bpf.h:6:
        /usr/include/linux/errno.h:1:10: fatal error: 'asm/errno.h' file not found
        #include <asm/errno.h>
                 ^~~~~~~~~~~~~
        1 error generated.

on my Fedora <asm/errno.h> is under /usr/include, so we should still get it from the standard include paths.

atenart commented 1 year ago

Maybe the comment on this one is a bit misleading (for me), it says Taking errno.h from libc instead of linux headers.

IMHO your commit makes sense, as we can have different paths on different distributions. But I'd just like to check the original change adding this path is working as expected. @amorenoz could you check?

amorenoz commented 1 year ago

Maybe the comment on this one is a bit misleading (for me), it says Taking errno.h from libc instead of linux headers.

It's true that it's only a workaround for the CI to work and it's not the path where errno.h is being taken in our (mostly Fedora) build machines. For debian systems it is a libc header:

> dpkg -S /usr/include/x86_64-linux-gnu/asm/errno.h
linux-libc-dev:amd64: /usr/include/x86_64-linux-gnu/asm/errno.h

We could also install the linux headers, which is more complicated in the CI VM because it's not using a stock kernel. In our systems it's most likely taken from kernel-headers (i.e: /usr/include/).

IMHO your commit makes sense, as we can have different paths on different distributions. But I'd just like to check the original change adding this path is working as expected. @amorenoz could you check?

In order to accommodate for more distros maybe the fix would be to only add to the include paths the right path. Maybe the first existing path in a list of optional/excluding paths? In any case, and since this is a temporary workaround anyway (we have to add proper header dependencies soon) I'm also OK with this patch

atenart commented 1 year ago

Thanks for the explanations @amorenoz.