Makefile: Update variables for package builds with external artifacts

[martinetd]

Hi!

I'm still using retsnoop once in a while and needed it at home (nixos), so figured I'd package it there.

First commit is mostly unrelated -- the link command ought to use LDFLAGS and not CFLAGS. Keeping CFLAGS doesn't really hurt but shouldn't be required so I removed it, happy to use both flags instead as long as LDFLAGS are added.

The second commit is a bit more complicated: Nixos requires the build to not use internet, so I need to build the addr2line sidecar crate with a frozen cargo command independently of the rest -- that's internal nixos mess and I'm not expecting any help here, but I also had problem with the sidecar ld command in the past (#31 -- actually that'd need an extra subst for - -> _, I wonder if make has a better syntax for this, sure couldn't find it... At least that can be overwritten with an env var now), so the SIDECAR_LDPATH might make that a bit simpler to use and at least ought to clarify where that magic symbol comes from.

Similarly, nixos libbpf package provides a libbpf.a, so we're more or less supposed to use it unless there's a very good reason not to -- the libbpf package is kept up to date and while the submodule is a bit more up to date than 1.3.0 I haven't seen any problem with the latest release so far.
I know what you're thinking about not using the submodule and I'm sure that'll come bite me at some point, but I'm expecting libbpf to have stabilized enough to be manageable since 1.0, so the worst that can happen is probably me bugging you for a new libbpf release if you tag a new retsnoop release that requires a new call from libbpf, and libbpf itself isn't getting updated. Hopefully not too often :)

The last commit is complete garbage, but I couldn't come up with a way to flag that I don't want to rebuild the sidecar (the :: make target will always try to run cargo and that'll fail for me); I think I'll just patch it on the nixos side but bringing it up here if you have a better idea.

With all of this, I can build retsnoop by just setting a few env vars as follow (after building addr2line and copying it to the current directory):

    LIBBPF_OBJ=${libbpf}/lib/libbpf.a \
      BPFTOOL=${lib.getExe bpftool} \
      SIDECAR=addr2line \
      make

There's no hurry on anything here, just tell me what you think we can keep when you have a moment and I'll clean up a bit, and I'll submit the nixos package when that's done.

[martinetd]

(huh apparently the static build test would need to update LDFLAGS to include --static now, but the bpftool dependency build doesn't like that, so that'd need to first build bpftool without LDFLAGS set and then build retsnoop with LDFLAGS set... I guess that can wait a bit as this isn't in mergeable state anyway; I'll do that some other day)

[anakryiko]

Hi!

I'm still using retsnoop once in a while and needed it at home (nixos), so figured I'd package it there.

Glad it's useful. I'm planning to do a bunch more work on retsnoop in the coming months, have been putting this off for a while while busy with other work stuff.

First commit is mostly unrelated -- the link command ought to use LDFLAGS and not CFLAGS. Keeping CFLAGS doesn't really hurt but shouldn't be required so I removed it, happy to use both flags instead as long as LDFLAGS are added.

Let's keep both CFLAGS and LDFLAGS. Maybe also submit this as a separate PR with a proper commit message?

The second commit is a bit more complicated: Nixos requires the build to not use internet, so I need to build the addr2line sidecar crate with a frozen cargo command independently of the rest -- that's internal nixos mess and I'm not expecting any help here, but I also had problem with the sidecar ld command in the past (#31 -- actually that'd need an extra subst for - -> _, I wonder if make has a better syntax for this, sure couldn't find it... At least that can be overwritten with an env var now), so the SIDECAR_LDPATH might make that a bit simpler to use and at least ought to clarify where that magic symbol comes from.

I think I'm fine with this, just SIDECAR_LDPATH name doesn't seem right, it's not LDPATH. Maybe SIDECAR_EMBED_NAME or something like that?

Similarly, nixos libbpf package provides a libbpf.a, so we're more or less supposed to use it unless there's a very good reason not to -- the libbpf package is kept up to date and while the submodule is a bit more up to date than 1.3.0 I haven't seen any problem with the latest release so far. I know what you're thinking about not using the submodule and I'm sure that'll come bite me at some point, but I'm expecting libbpf to have stabilized enough to be manageable since 1.0, so the worst that can happen is probably me bugging you for a new libbpf release if you tag a new retsnoop release that requires a new call from libbpf, and libbpf itself isn't getting updated. Hopefully not too often :)

Yes, I do occasionally need some libbpf feature that hasn't been released, so using libbpf outside of submodule sometimes might not work. If you are up to test and deal with these breakages in your specific setup, I don't see much problem to allow user to override libbpf.a location. It's still statically linked, so I'm fine with this.

The last commit is complete garbage, but I couldn't come up with a way to flag that I don't want to rebuild the sidecar (the :: make target will always try to run cargo and that'll fail for me); I think I'll just patch it on the nixos side but bringing it up here if you have a better idea.

yeah, this is not good for "normal" build path. What we can do, perhaps, is to have DEFAULT_SIDECAR vs SIDECAR that can be overridden, and if they don't match, don't even trigger cargo (as it's pointless anyways). Let's do this as a separate PR still?

With all of this, I can build retsnoop by just setting a few env vars as follow (after building addr2line and copying it to the current directory):

    LIBBPF_OBJ=${libbpf}/lib/libbpf.a \
      BPFTOOL=${lib.getExe bpftool} \
      SIDECAR=addr2line \
      make

There's no hurry on anything here, just tell me what you think we can keep when you have a moment and I'll clean up a bit, and I'll submit the nixos package when that's done.

Yep, let's split three changes and review/discuss each individually.

[martinetd]

Thanks for the reply!

I've split the commits and applied suggestions. Replying to the parts concering the commit left here below.

I think I'm fine with this, just SIDECAR_LDPATH name doesn't seem right, it's not LDPATH. Maybe SIDECAR_EMBED_NAME or something like that?

Right, I looked through the ld documentation and was unable to find a reference to this behaviour (very little about -b binary, and nothing about _binary_${path}_start/end/size); digging through the code is clear enough that it's the name of the input file with non-alphanumeric characters changed to underscore but that doesn't really help naming this... it's called mangled_name in the code and just FILENAME in a comment.
I'm not fussy here anyway, I've updated it to SIDECAR_EMBED_NAME.

Yes, I do occasionally need some libbpf feature that hasn't been released, so using libbpf outside of submodule sometimes might not work. If you are up to test and deal with these breakages in your specific setup, I don't see much problem to allow user to override libbpf.a location. It's still statically linked, so I'm fine with this.

This makes sense -- I'll add some test to make sure a basic trace works when updating either libbpf or retsnoop when packaging it.