diff mbox series

package/bpftool: add a patch to fix cross compilation

Message ID 48ac54a1-f946-8f98-42e4-e39d2ab7130d@synopsys.com
State Accepted
Headers show
Series package/bpftool: add a patch to fix cross compilation | expand

Commit Message

Shahab Vahedi June 10, 2022, 1:33 p.m. UTC 
If on the host machine the "co-re" is supported, bpftool will
build a bootstrap version of itself as well. In that case, the
cross compilation can fail. This commit adds a patch to remedy
that. The fix that you see here is already upsteamed [1].

[1]
https://lore.kernel.org/bpf/165477661272.11342.13015777410417612477.git-patchwork-notify@kernel.org/T/#t

Signed-off-by: Shahab Vahedi <shahab@synopsys.com>
---
 package/bpftool/0001-fix-bootsrap.patch | 32 +++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 package/bpftool/0001-fix-bootsrap.patch

Comments

Arnout Vandecappelle June 13, 2022, 8:21 p.m. UTC | #1 
On 10/06/2022 15:33, Shahab Vahedi via buildroot wrote:
> If on the host machine the "co-re" is supported, bpftool will
> build a bootstrap version of itself as well. In that case, the
> cross compilation can fail. This commit adds a patch to remedy
> that. The fix that you see here is already upsteamed [1].
> 
> [1]
> https://lore.kernel.org/bpf/165477661272.11342.13015777410417612477.git-patchwork-notify@kernel.org/T/#t
> 
> Signed-off-by: Shahab Vahedi <shahab@synopsys.com>
> ---
>   package/bpftool/0001-fix-bootsrap.patch | 32 +++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>   create mode 100644 package/bpftool/0001-fix-bootsrap.patch
> 
> diff --git a/package/bpftool/0001-fix-bootsrap.patch b/package/bpftool/0001-fix-bootsrap.patch
> new file mode 100644
> index 0000000000..03f4231cca
> --- /dev/null
> +++ b/package/bpftool/0001-fix-bootsrap.patch
> @@ -0,0 +1,32 @@
> +This is an adapted version of an upstreamed patch [1], else it won't be
> +possible to cross compile bpftool if "clang-bpf-co-re" feature is enabled.
> +
> +[1] bpftool: Fix bootstrapping during a cross compilation
> +https://lore.kernel.org/bpf/8d297f0c-cfd0-ef6f-3970-6dddb3d9a87a@synopsys.com/t/#u

  The patch file should be git-formatted and have your Signed-off-by. I simply 
took the patch from [1] (which has your signoff) and added the additional 
Makefile.include changes (of which I don't really understand BTW why they're 
relevant for Buildroot).


  Regards,
  Arnout

[1] 
https://github.com/libbpf/bpftool/commit/189f777ea4829bede0bf92f572c22fe1f2c37522


> +--- bpftool-v6.8.0/src/Makefile
> ++++ bpftool-v6.8.0/src/Makefile
> +@@ -51,7 +51,7 @@
> + $(LIBBPF_BOOTSTRAP): $(wildcard $(BPF_DIR)/*.[ch] $(BPF_DIR)/Makefile) | $(LIBBPF_BOOTSTRAP_OUTPUT)
> + 	$(Q)$(MAKE) -C $(BPF_DIR) OBJDIR=$(patsubst %/,%,$(LIBBPF_BOOTSTRAP_OUTPUT)) \
> + 		PREFIX=$(LIBBPF_BOOTSTRAP_DESTDIR:/=) \
> +-		ARCH= CROSS_COMPILE= CC=$(HOSTCC) LD=$(HOSTLD) $@ install_headers
> ++		ARCH= CROSS_COMPILE= CC=$(HOSTCC) LD=$(HOSTLD) AR=$(HOSTAR) $@ install_headers
> +
> + $(LIBBPF_BOOTSTRAP_INTERNAL_HDRS): $(LIBBPF_BOOTSTRAP_HDRS_DIR)/%.h: $(BPF_DIR)/%.h | $(LIBBPF_BOOTSTRAP_HDRS_DIR)
> + 	$(call QUIET_INSTALL, $@)
> +--- bpftool-v6.8.0/src/Makefile.include
> ++++ bpftool-v6.8.0/src/Makefile.include
> +@@ -12,11 +12,13 @@
> + ifneq ($(LLVM),)
> +   $(if $(findstring default,$(origin CC)),$(eval CC := clang$(LLVM_VERSION)))
> +   $(if $(findstring default,$(origin LD)),$(eval LD := ld.lld$(LLVM_VERSION)))
> ++  HOSTAR ?= llvm-ar
> +   HOSTCC ?= clang
> +   HOSTLD ?= ld.lld
> + else
> +   $(if $(findstring default,$(origin CC)),$(eval CC = $(CROSS_COMPILE)$(CC)))
> +   $(if $(findstring default,$(origin LD)),$(eval LD = $(CROSS_COMPILE)$(LD)))
> ++  HOSTAR ?= ar
> +   HOSTCC ?= gcc
> +   HOSTLD ?= ld
> + endif
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
Shahab Vahedi June 13, 2022, 8:48 p.m. UTC | #2 
Arnout,

On 6/13/22 22:21, Arnout Vandecappelle wrote:
>> The patch file should be git-formatted and have your Signed-off-by.
> I simply took the patch from [1] (which has your signoff) and added
> the additional Makefile.include changes

Thanks for the tweak. I overlooked the "Sign-off" part. I used "diff"
because of what BR's manual suggests [1]:

  If the software is under version control, it is recommended to use
  the upstream SCM software to generate the patch set.

  Otherwise, concatenate the header with the output of the
  diff -purN ...

The proposed patch was made for a tar ball release that is not under
any version control per se. With the hindsight, maybe I should have
checked for "v6.8.0" as branch/tag in upstream repo.

> (of which I don't really understand BTW why they're relevant for Buildroot).

That extra change (setting "HOSTAR") is necessary, because in upstream
that was already taken care of before the patch submission. However, in
v6.8.0 release "HOSTAR" is never initialised and the fix is actually
using it.

[1]
https://buildroot.org/downloads/manual/manual.html#_format_and_licensing_of_the_package_patches


Cheers,
Shahab
Arnout Vandecappelle June 14, 2022, 5:04 p.m. UTC | #3 
On 13/06/2022 22:48, Shahab Vahedi wrote:
> Arnout,
> 
> On 6/13/22 22:21, Arnout Vandecappelle wrote:
>>> The patch file should be git-formatted and have your Signed-off-by.
>> I simply took the patch from [1] (which has your signoff) and added
>> the additional Makefile.include changes
> 
> Thanks for the tweak. I overlooked the "Sign-off" part. I used "diff"
> because of what BR's manual suggests [1]:
> 
>    If the software is under version control, it is recommended to use
>    the upstream SCM software to generate the patch set.
> 
>    Otherwise, concatenate the header with the output of the
>    diff -purN ...
> 
> The proposed patch was made for a tar ball release that is not under
> any version control per se. With the hindsight, maybe I should have
> checked for "v6.8.0" as branch/tag in upstream repo.

  Yeah, bpftool is actually a special situation since the real upstream (where 
you contributed your patch) is a separate repository that gets regularly synced 
into the bpftool repo. I think that at the time you sent this to Buildroot, it 
was not yet synced to bpftool.

>> (of which I don't really understand BTW why they're relevant for Buildroot).
> 
> That extra change (setting "HOSTAR") is necessary, because in upstream
> that was already taken care of before the patch submission. However, in
> v6.8.0 release "HOSTAR" is never initialised and the fix is actually
> using it.

  Since we set it from the .mk file, it doesn't actually *need* to be 
initialized in Buildroot context.

  Also, if that came from a previously applied upstream patch, it's better to 
backport the actual upstream patch. But that can be complicated as well for 
something like bpftool, so it's fully understandable to do it like this. It 
would be nice however in such a case to add a reference to the upstream commit 
that added it.

  Regards,
  Arnout

> 
> [1]
> https://buildroot.org/downloads/manual/manual.html#_format_and_licensing_of_the_package_patches
> 
> 
> Cheers,
> Shahab
Peter Korsgaard July 11, 2022, 4:56 p.m. UTC | #4 
On 13/06/2022 22.21, Arnout Vandecappelle wrote:
> 
> 
> On 10/06/2022 15:33, Shahab Vahedi via buildroot wrote:
>> If on the host machine the "co-re" is supported, bpftool will
>> build a bootstrap version of itself as well. In that case, the
>> cross compilation can fail. This commit adds a patch to remedy
>> that. The fix that you see here is already upsteamed [1].
>>
>> [1]
>> https://lore.kernel.org/bpf/165477661272.11342.13015777410417612477.git-patchwork-notify@kernel.org/T/#t 
>>
>>
>> Signed-off-by: Shahab Vahedi <shahab@synopsys.com>
>> ---
>>   package/bpftool/0001-fix-bootsrap.patch | 32 +++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>   create mode 100644 package/bpftool/0001-fix-bootsrap.patch
>>
>> diff --git a/package/bpftool/0001-fix-bootsrap.patch 
>> b/package/bpftool/0001-fix-bootsrap.patch
>> new file mode 100644
>> index 0000000000..03f4231cca
>> --- /dev/null
>> +++ b/package/bpftool/0001-fix-bootsrap.patch
>> @@ -0,0 +1,32 @@
>> +This is an adapted version of an upstreamed patch [1], else it won't be
>> +possible to cross compile bpftool if "clang-bpf-co-re" feature is 
>> enabled.
>> +
>> +[1] bpftool: Fix bootstrapping during a cross compilation
>> +https://lore.kernel.org/bpf/8d297f0c-cfd0-ef6f-3970-6dddb3d9a87a@synopsys.com/t/#u 
>>
> 
>   The patch file should be git-formatted and have your Signed-off-by. I 
> simply took the patch from [1] (which has your signoff) and added the 
> additional Makefile.include changes (of which I don't really understand 
> BTW why they're relevant for Buildroot).

Committed to 2022.02.x and 2022.05.x, thanks.
diff mbox series

Patch

diff --git a/package/bpftool/0001-fix-bootsrap.patch b/package/bpftool/0001-fix-bootsrap.patch
new file mode 100644
index 0000000000..03f4231cca
--- /dev/null
+++ b/package/bpftool/0001-fix-bootsrap.patch
@@ -0,0 +1,32 @@ 
+This is an adapted version of an upstreamed patch [1], else it won't be
+possible to cross compile bpftool if "clang-bpf-co-re" feature is enabled.
+
+[1] bpftool: Fix bootstrapping during a cross compilation
+https://lore.kernel.org/bpf/8d297f0c-cfd0-ef6f-3970-6dddb3d9a87a@synopsys.com/t/#u
+--- bpftool-v6.8.0/src/Makefile
++++ bpftool-v6.8.0/src/Makefile
+@@ -51,7 +51,7 @@
+ $(LIBBPF_BOOTSTRAP): $(wildcard $(BPF_DIR)/*.[ch] $(BPF_DIR)/Makefile) | $(LIBBPF_BOOTSTRAP_OUTPUT)
+ 	$(Q)$(MAKE) -C $(BPF_DIR) OBJDIR=$(patsubst %/,%,$(LIBBPF_BOOTSTRAP_OUTPUT)) \
+ 		PREFIX=$(LIBBPF_BOOTSTRAP_DESTDIR:/=) \
+-		ARCH= CROSS_COMPILE= CC=$(HOSTCC) LD=$(HOSTLD) $@ install_headers
++		ARCH= CROSS_COMPILE= CC=$(HOSTCC) LD=$(HOSTLD) AR=$(HOSTAR) $@ install_headers
+ 
+ $(LIBBPF_BOOTSTRAP_INTERNAL_HDRS): $(LIBBPF_BOOTSTRAP_HDRS_DIR)/%.h: $(BPF_DIR)/%.h | $(LIBBPF_BOOTSTRAP_HDRS_DIR)
+ 	$(call QUIET_INSTALL, $@)
+--- bpftool-v6.8.0/src/Makefile.include
++++ bpftool-v6.8.0/src/Makefile.include
+@@ -12,11 +12,13 @@
+ ifneq ($(LLVM),)
+   $(if $(findstring default,$(origin CC)),$(eval CC := clang$(LLVM_VERSION)))
+   $(if $(findstring default,$(origin LD)),$(eval LD := ld.lld$(LLVM_VERSION)))
++  HOSTAR ?= llvm-ar
+   HOSTCC ?= clang
+   HOSTLD ?= ld.lld
+ else
+   $(if $(findstring default,$(origin CC)),$(eval CC = $(CROSS_COMPILE)$(CC)))
+   $(if $(findstring default,$(origin LD)),$(eval LD = $(CROSS_COMPILE)$(LD)))
++  HOSTAR ?= ar
+   HOSTCC ?= gcc
+   HOSTLD ?= ld
+ endif