mbox series

[bpf-next,v2,00/10] tools: Use consistent libbpf include paths everywhere

Message ID 157909756858.1192265.6657542187065456112.stgit@toke.dk (mailing list archive)
Headers show
Series tools: Use consistent libbpf include paths everywhere | expand

Message

Toke Høiland-Jørgensen Jan. 15, 2020, 2:12 p.m. UTC
The recent commit 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are
taken from selftests dir") broke compilation against libbpf if it is installed
on the system, and $INCLUDEDIR/bpf is not in the include path.

Since having the bpf/ subdir of $INCLUDEDIR in the include path has never been a
requirement for building against libbpf before, this needs to be fixed. One
option is to just revert the offending commit and figure out a different way to
achieve what it aims for. However, this series takes a different approach:
Changing all in-tree users of libbpf to consistently use a bpf/ prefix in
#include directives for header files from libbpf.

This turns out to be a somewhat invasive change in the number of files touched;
however, the actual changes to files are fairly trivial (most of them are simply
made with 'sed'). Also, this approach has the advantage that it makes external
and internal users consistent with each other, and ensures no future changes
breaks things in the same way as the commit referenced above.

The series is split to make the change for one tool subdir at a time, while
trying not to break the build along the way. It is structured like this:

- Patch 1-2: Trivial fixes to Makefiles for issues I discovered while changing
  the include paths.

- Patch 3-7: Change the include directives to use the bpf/ prefix, and updates
  Makefiles to make sure tools/lib/ is part of the include path, but without
  removing tools/lib/bpf

- Patch 8: Change the bpf_helpers file in libbpf itself to use the bpf/ prefix
  when including (the original source of breakage).

- Patch 9-10: Remove tools/lib/bpf from include paths to make sure we don't
  inadvertently re-introduce includes without the bpf/ prefix.

---

Toke Høiland-Jørgensen (10):
      samples/bpf: Don't try to remove user's homedir on clean
      tools/bpf/runqslower: Fix override option for VMLINUX_BTF
      tools/runqslower: Use consistent include paths for libbpf
      selftests: Use consistent include paths for libbpf
      bpftool: Use consistent include paths for libbpf
      perf: Use consistent include paths for libbpf
      samples/bpf: Use consistent include paths for libbpf
      libbpf: Fix include of bpf_helpers.h when libbpf is installed on system
      selftests: Remove tools/lib/bpf from include path
      tools/runqslower: Remove tools/lib/bpf from include path


 samples/bpf/Makefile                               |    5 ++---
 samples/bpf/cpustat_kern.c                         |    2 +-
 samples/bpf/fds_example.c                          |    2 +-
 samples/bpf/hbm.c                                  |    4 ++--
 samples/bpf/hbm_kern.h                             |    4 ++--
 samples/bpf/ibumad_kern.c                          |    2 +-
 samples/bpf/ibumad_user.c                          |    2 +-
 samples/bpf/lathist_kern.c                         |    2 +-
 samples/bpf/lwt_len_hist_kern.c                    |    2 +-
 samples/bpf/map_perf_test_kern.c                   |    4 ++--
 samples/bpf/offwaketime_kern.c                     |    4 ++--
 samples/bpf/offwaketime_user.c                     |    2 +-
 samples/bpf/parse_ldabs.c                          |    2 +-
 samples/bpf/parse_simple.c                         |    2 +-
 samples/bpf/parse_varlen.c                         |    2 +-
 samples/bpf/sampleip_kern.c                        |    4 ++--
 samples/bpf/sampleip_user.c                        |    2 +-
 samples/bpf/sock_flags_kern.c                      |    2 +-
 samples/bpf/sockex1_kern.c                         |    2 +-
 samples/bpf/sockex1_user.c                         |    2 +-
 samples/bpf/sockex2_kern.c                         |    2 +-
 samples/bpf/sockex2_user.c                         |    2 +-
 samples/bpf/sockex3_kern.c                         |    2 +-
 samples/bpf/spintest_kern.c                        |    4 ++--
 samples/bpf/spintest_user.c                        |    2 +-
 samples/bpf/syscall_tp_kern.c                      |    2 +-
 samples/bpf/task_fd_query_kern.c                   |    2 +-
 samples/bpf/task_fd_query_user.c                   |    2 +-
 samples/bpf/tc_l2_redirect_kern.c                  |    2 +-
 samples/bpf/tcbpf1_kern.c                          |    2 +-
 samples/bpf/tcp_basertt_kern.c                     |    4 ++--
 samples/bpf/tcp_bufs_kern.c                        |    4 ++--
 samples/bpf/tcp_clamp_kern.c                       |    4 ++--
 samples/bpf/tcp_cong_kern.c                        |    4 ++--
 samples/bpf/tcp_dumpstats_kern.c                   |    4 ++--
 samples/bpf/tcp_iw_kern.c                          |    4 ++--
 samples/bpf/tcp_rwnd_kern.c                        |    4 ++--
 samples/bpf/tcp_synrto_kern.c                      |    4 ++--
 samples/bpf/tcp_tos_reflect_kern.c                 |    4 ++--
 samples/bpf/test_cgrp2_tc_kern.c                   |    2 +-
 samples/bpf/test_current_task_under_cgroup_kern.c  |    2 +-
 samples/bpf/test_lwt_bpf.c                         |    2 +-
 samples/bpf/test_map_in_map_kern.c                 |    4 ++--
 samples/bpf/test_overhead_kprobe_kern.c            |    4 ++--
 samples/bpf/test_overhead_raw_tp_kern.c            |    2 +-
 samples/bpf/test_overhead_tp_kern.c                |    2 +-
 samples/bpf/test_probe_write_user_kern.c           |    4 ++--
 samples/bpf/trace_event_kern.c                     |    4 ++--
 samples/bpf/trace_event_user.c                     |    2 +-
 samples/bpf/trace_output_kern.c                    |    2 +-
 samples/bpf/trace_output_user.c                    |    2 +-
 samples/bpf/tracex1_kern.c                         |    4 ++--
 samples/bpf/tracex2_kern.c                         |    4 ++--
 samples/bpf/tracex3_kern.c                         |    4 ++--
 samples/bpf/tracex4_kern.c                         |    4 ++--
 samples/bpf/tracex5_kern.c                         |    4 ++--
 samples/bpf/tracex6_kern.c                         |    2 +-
 samples/bpf/tracex7_kern.c                         |    2 +-
 samples/bpf/xdp1_kern.c                            |    2 +-
 samples/bpf/xdp1_user.c                            |    4 ++--
 samples/bpf/xdp2_kern.c                            |    2 +-
 samples/bpf/xdp2skb_meta_kern.c                    |    2 +-
 samples/bpf/xdp_adjust_tail_kern.c                 |    2 +-
 samples/bpf/xdp_adjust_tail_user.c                 |    4 ++--
 samples/bpf/xdp_fwd_kern.c                         |    2 +-
 samples/bpf/xdp_fwd_user.c                         |    2 +-
 samples/bpf/xdp_monitor_kern.c                     |    2 +-
 samples/bpf/xdp_redirect_cpu_kern.c                |    2 +-
 samples/bpf/xdp_redirect_cpu_user.c                |    2 +-
 samples/bpf/xdp_redirect_kern.c                    |    2 +-
 samples/bpf/xdp_redirect_map_kern.c                |    2 +-
 samples/bpf/xdp_redirect_map_user.c                |    2 +-
 samples/bpf/xdp_redirect_user.c                    |    2 +-
 samples/bpf/xdp_router_ipv4_kern.c                 |    2 +-
 samples/bpf/xdp_router_ipv4_user.c                 |    2 +-
 samples/bpf/xdp_rxq_info_kern.c                    |    2 +-
 samples/bpf/xdp_rxq_info_user.c                    |    4 ++--
 samples/bpf/xdp_sample_pkts_kern.c                 |    2 +-
 samples/bpf/xdp_sample_pkts_user.c                 |    2 +-
 samples/bpf/xdp_tx_iptunnel_kern.c                 |    2 +-
 samples/bpf/xdp_tx_iptunnel_user.c                 |    2 +-
 samples/bpf/xdpsock_kern.c                         |    2 +-
 samples/bpf/xdpsock_user.c                         |    6 +++---
 tools/bpf/bpftool/Documentation/bpftool-gen.rst    |    2 +-
 tools/bpf/bpftool/Makefile                         |    2 +-
 tools/bpf/bpftool/btf.c                            |    8 ++++----
 tools/bpf/bpftool/btf_dumper.c                     |    2 +-
 tools/bpf/bpftool/cgroup.c                         |    2 +-
 tools/bpf/bpftool/common.c                         |    4 ++--
 tools/bpf/bpftool/feature.c                        |    4 ++--
 tools/bpf/bpftool/gen.c                            |   10 +++++-----
 tools/bpf/bpftool/jit_disasm.c                     |    2 +-
 tools/bpf/bpftool/main.c                           |    4 ++--
 tools/bpf/bpftool/map.c                            |    4 ++--
 tools/bpf/bpftool/map_perf_ring.c                  |    4 ++--
 tools/bpf/bpftool/net.c                            |    8 ++++----
 tools/bpf/bpftool/netlink_dumper.c                 |    4 ++--
 tools/bpf/bpftool/perf.c                           |    2 +-
 tools/bpf/bpftool/prog.c                           |    6 +++---
 tools/bpf/bpftool/xlated_dumper.c                  |    2 +-
 tools/bpf/runqslower/Makefile                      |   21 ++++++++++++--------
 tools/bpf/runqslower/runqslower.bpf.c              |    2 +-
 tools/bpf/runqslower/runqslower.c                  |    4 ++--
 tools/lib/bpf/bpf_helpers.h                        |    2 +-
 tools/perf/examples/bpf/5sec.c                     |    2 +-
 tools/perf/examples/bpf/empty.c                    |    2 +-
 tools/perf/examples/bpf/sys_enter_openat.c         |    2 +-
 tools/perf/include/bpf/pid_filter.h                |    2 +-
 tools/perf/include/bpf/stdio.h                     |    2 +-
 tools/perf/include/bpf/unistd.h                    |    2 +-
 tools/testing/selftests/bpf/.gitignore             |    3 ++-
 tools/testing/selftests/bpf/Makefile               |   16 ++++++++-------
 tools/testing/selftests/bpf/bpf_tcp_helpers.h      |    4 ++--
 tools/testing/selftests/bpf/bpf_trace_helpers.h    |    2 +-
 tools/testing/selftests/bpf/bpf_util.h             |    2 +-
 tools/testing/selftests/bpf/prog_tests/cpu_mask.c  |    2 +-
 .../testing/selftests/bpf/prog_tests/perf_buffer.c |    2 +-
 tools/testing/selftests/bpf/progs/bpf_dctcp.c      |    2 +-
 tools/testing/selftests/bpf/progs/bpf_flow.c       |    4 ++--
 tools/testing/selftests/bpf/progs/connect4_prog.c  |    4 ++--
 tools/testing/selftests/bpf/progs/connect6_prog.c  |    4 ++--
 tools/testing/selftests/bpf/progs/dev_cgroup.c     |    2 +-
 tools/testing/selftests/bpf/progs/fentry_test.c    |    2 +-
 tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c  |    2 +-
 .../selftests/bpf/progs/fexit_bpf2bpf_simple.c     |    2 +-
 tools/testing/selftests/bpf/progs/fexit_test.c     |    2 +-
 .../selftests/bpf/progs/get_cgroup_id_kern.c       |    2 +-
 tools/testing/selftests/bpf/progs/kfree_skb.c      |    4 ++--
 tools/testing/selftests/bpf/progs/loop1.c          |    4 ++--
 tools/testing/selftests/bpf/progs/loop2.c          |    4 ++--
 tools/testing/selftests/bpf/progs/loop3.c          |    4 ++--
 tools/testing/selftests/bpf/progs/loop4.c          |    2 +-
 tools/testing/selftests/bpf/progs/loop5.c          |    2 +-
 tools/testing/selftests/bpf/progs/netcnt_prog.c    |    2 +-
 tools/testing/selftests/bpf/progs/pyperf.h         |    2 +-
 .../testing/selftests/bpf/progs/sample_map_ret0.c  |    2 +-
 tools/testing/selftests/bpf/progs/sendmsg4_prog.c  |    4 ++--
 tools/testing/selftests/bpf/progs/sendmsg6_prog.c  |    4 ++--
 .../selftests/bpf/progs/socket_cookie_prog.c       |    4 ++--
 .../selftests/bpf/progs/sockmap_parse_prog.c       |    4 ++--
 .../selftests/bpf/progs/sockmap_tcp_msg_prog.c     |    4 ++--
 .../selftests/bpf/progs/sockmap_verdict_prog.c     |    4 ++--
 .../testing/selftests/bpf/progs/sockopt_inherit.c  |    2 +-
 tools/testing/selftests/bpf/progs/sockopt_multi.c  |    2 +-
 tools/testing/selftests/bpf/progs/sockopt_sk.c     |    2 +-
 tools/testing/selftests/bpf/progs/strobemeta.h     |    2 +-
 tools/testing/selftests/bpf/progs/tailcall1.c      |    2 +-
 tools/testing/selftests/bpf/progs/tailcall2.c      |    2 +-
 tools/testing/selftests/bpf/progs/tailcall3.c      |    2 +-
 tools/testing/selftests/bpf/progs/tailcall4.c      |    2 +-
 tools/testing/selftests/bpf/progs/tailcall5.c      |    2 +-
 tools/testing/selftests/bpf/progs/tcp_rtt.c        |    2 +-
 .../testing/selftests/bpf/progs/test_adjust_tail.c |    2 +-
 .../selftests/bpf/progs/test_attach_probe.c        |    2 +-
 tools/testing/selftests/bpf/progs/test_btf_haskv.c |    2 +-
 tools/testing/selftests/bpf/progs/test_btf_newkv.c |    2 +-
 tools/testing/selftests/bpf/progs/test_btf_nokv.c  |    2 +-
 .../testing/selftests/bpf/progs/test_core_extern.c |    2 +-
 .../selftests/bpf/progs/test_core_reloc_arrays.c   |    4 ++--
 .../bpf/progs/test_core_reloc_bitfields_direct.c   |    4 ++--
 .../bpf/progs/test_core_reloc_bitfields_probed.c   |    4 ++--
 .../bpf/progs/test_core_reloc_existence.c          |    4 ++--
 .../selftests/bpf/progs/test_core_reloc_flavors.c  |    4 ++--
 .../selftests/bpf/progs/test_core_reloc_ints.c     |    4 ++--
 .../selftests/bpf/progs/test_core_reloc_kernel.c   |    4 ++--
 .../selftests/bpf/progs/test_core_reloc_misc.c     |    4 ++--
 .../selftests/bpf/progs/test_core_reloc_mods.c     |    4 ++--
 .../selftests/bpf/progs/test_core_reloc_nesting.c  |    4 ++--
 .../bpf/progs/test_core_reloc_primitives.c         |    4 ++--
 .../bpf/progs/test_core_reloc_ptr_as_arr.c         |    4 ++--
 .../selftests/bpf/progs/test_core_reloc_size.c     |    4 ++--
 .../selftests/bpf/progs/test_get_stack_rawtp.c     |    2 +-
 .../testing/selftests/bpf/progs/test_global_data.c |    2 +-
 .../selftests/bpf/progs/test_global_func1.c        |    2 +-
 .../selftests/bpf/progs/test_global_func3.c        |    2 +-
 .../selftests/bpf/progs/test_global_func5.c        |    2 +-
 .../selftests/bpf/progs/test_global_func6.c        |    2 +-
 .../selftests/bpf/progs/test_global_func7.c        |    2 +-
 tools/testing/selftests/bpf/progs/test_l4lb.c      |    4 ++--
 .../selftests/bpf/progs/test_l4lb_noinline.c       |    4 ++--
 .../selftests/bpf/progs/test_lirc_mode2_kern.c     |    2 +-
 .../selftests/bpf/progs/test_lwt_ip_encap.c        |    4 ++--
 .../selftests/bpf/progs/test_lwt_seg6local.c       |    4 ++--
 .../testing/selftests/bpf/progs/test_map_in_map.c  |    2 +-
 tools/testing/selftests/bpf/progs/test_map_lock.c  |    2 +-
 tools/testing/selftests/bpf/progs/test_mmap.c      |    2 +-
 tools/testing/selftests/bpf/progs/test_obj_id.c    |    2 +-
 tools/testing/selftests/bpf/progs/test_overhead.c  |    4 ++--
 .../testing/selftests/bpf/progs/test_perf_buffer.c |    2 +-
 tools/testing/selftests/bpf/progs/test_pinning.c   |    2 +-
 .../selftests/bpf/progs/test_pinning_invalid.c     |    2 +-
 .../testing/selftests/bpf/progs/test_pkt_access.c  |    4 ++--
 .../selftests/bpf/progs/test_pkt_md_access.c       |    2 +-
 .../testing/selftests/bpf/progs/test_probe_user.c  |    4 ++--
 .../selftests/bpf/progs/test_queue_stack_map.h     |    2 +-
 .../testing/selftests/bpf/progs/test_rdonly_maps.c |    2 +-
 tools/testing/selftests/bpf/progs/test_seg6_loop.c |    4 ++--
 .../bpf/progs/test_select_reuseport_kern.c         |    4 ++--
 .../selftests/bpf/progs/test_send_signal_kern.c    |    2 +-
 .../selftests/bpf/progs/test_sk_lookup_kern.c      |    4 ++--
 .../selftests/bpf/progs/test_skb_cgroup_id_kern.c  |    2 +-
 tools/testing/selftests/bpf/progs/test_skb_ctx.c   |    2 +-
 tools/testing/selftests/bpf/progs/test_skeleton.c  |    2 +-
 .../selftests/bpf/progs/test_sock_fields_kern.c    |    4 ++--
 tools/testing/selftests/bpf/progs/test_spin_lock.c |    2 +-
 .../selftests/bpf/progs/test_stacktrace_build_id.c |    2 +-
 .../selftests/bpf/progs/test_stacktrace_map.c      |    2 +-
 .../selftests/bpf/progs/test_sysctl_loop1.c        |    2 +-
 .../selftests/bpf/progs/test_sysctl_loop2.c        |    2 +-
 .../testing/selftests/bpf/progs/test_sysctl_prog.c |    2 +-
 tools/testing/selftests/bpf/progs/test_tc_edt.c    |    4 ++--
 tools/testing/selftests/bpf/progs/test_tc_tunnel.c |    4 ++--
 .../bpf/progs/test_tcp_check_syncookie_kern.c      |    4 ++--
 .../testing/selftests/bpf/progs/test_tcp_estats.c  |    2 +-
 .../testing/selftests/bpf/progs/test_tcpbpf_kern.c |    4 ++--
 .../selftests/bpf/progs/test_tcpnotify_kern.c      |    4 ++--
 .../testing/selftests/bpf/progs/test_tracepoint.c  |    2 +-
 .../testing/selftests/bpf/progs/test_tunnel_kern.c |    4 ++--
 .../selftests/bpf/progs/test_verif_scale1.c        |    2 +-
 .../selftests/bpf/progs/test_verif_scale2.c        |    2 +-
 .../selftests/bpf/progs/test_verif_scale3.c        |    2 +-
 tools/testing/selftests/bpf/progs/test_xdp.c       |    4 ++--
 tools/testing/selftests/bpf/progs/test_xdp_loop.c  |    4 ++--
 tools/testing/selftests/bpf/progs/test_xdp_meta.c  |    2 +-
 .../selftests/bpf/progs/test_xdp_noinline.c        |    4 ++--
 .../selftests/bpf/progs/test_xdp_redirect.c        |    2 +-
 tools/testing/selftests/bpf/progs/test_xdp_vlan.c  |    4 ++--
 tools/testing/selftests/bpf/progs/xdp_dummy.c      |    2 +-
 .../testing/selftests/bpf/progs/xdp_redirect_map.c |    2 +-
 tools/testing/selftests/bpf/progs/xdp_tx.c         |    2 +-
 tools/testing/selftests/bpf/progs/xdping_kern.c    |    4 ++--
 tools/testing/selftests/bpf/test_cpp.cpp           |    6 +++---
 tools/testing/selftests/bpf/test_hashmap.c         |    2 +-
 tools/testing/selftests/bpf/test_progs.h           |    2 +-
 tools/testing/selftests/bpf/test_sock.c            |    2 +-
 tools/testing/selftests/bpf/test_sockmap_kern.h    |    4 ++--
 tools/testing/selftests/bpf/test_sysctl.c          |    2 +-
 tools/testing/selftests/bpf/trace_helpers.h        |    2 +-
 238 files changed, 359 insertions(+), 354 deletions(-)

Comments

Andrii Nakryiko Jan. 15, 2020, 6:06 p.m. UTC | #1
On Wed, Jan 15, 2020 at 6:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> The recent commit 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are
> taken from selftests dir") broke compilation against libbpf if it is installed
> on the system, and $INCLUDEDIR/bpf is not in the include path.
>
> Since having the bpf/ subdir of $INCLUDEDIR in the include path has never been a
> requirement for building against libbpf before, this needs to be fixed. One
> option is to just revert the offending commit and figure out a different way to
> achieve what it aims for. However, this series takes a different approach:
> Changing all in-tree users of libbpf to consistently use a bpf/ prefix in
> #include directives for header files from libbpf.
>
> This turns out to be a somewhat invasive change in the number of files touched;
> however, the actual changes to files are fairly trivial (most of them are simply
> made with 'sed'). Also, this approach has the advantage that it makes external
> and internal users consistent with each other, and ensures no future changes
> breaks things in the same way as the commit referenced above.
>
> The series is split to make the change for one tool subdir at a time, while
> trying not to break the build along the way. It is structured like this:
>
> - Patch 1-2: Trivial fixes to Makefiles for issues I discovered while changing
>   the include paths.
>
> - Patch 3-7: Change the include directives to use the bpf/ prefix, and updates
>   Makefiles to make sure tools/lib/ is part of the include path, but without
>   removing tools/lib/bpf
>
> - Patch 8: Change the bpf_helpers file in libbpf itself to use the bpf/ prefix
>   when including (the original source of breakage).
>
> - Patch 9-10: Remove tools/lib/bpf from include paths to make sure we don't
>   inadvertently re-introduce includes without the bpf/ prefix.
>
> ---

Thanks, Toke, for this clean up! I tested it locally for my set up:
runqslower, bpftool, libbpf, and selftests all build fine, so it looks
good. My only concern is with selftests/bpf Makefile, we shouldn't
build anything outside of selftests/bpf. Let's fix that. Thanks!

>
> Toke Høiland-Jørgensen (10):
>       samples/bpf: Don't try to remove user's homedir on clean
>       tools/bpf/runqslower: Fix override option for VMLINUX_BTF
>       tools/runqslower: Use consistent include paths for libbpf
>       selftests: Use consistent include paths for libbpf
>       bpftool: Use consistent include paths for libbpf
>       perf: Use consistent include paths for libbpf
>       samples/bpf: Use consistent include paths for libbpf
>       libbpf: Fix include of bpf_helpers.h when libbpf is installed on system
>       selftests: Remove tools/lib/bpf from include path
>       tools/runqslower: Remove tools/lib/bpf from include path
>
>

[...]
Alexei Starovoitov Jan. 15, 2020, 9:19 p.m. UTC | #2
On Wed, Jan 15, 2020 at 03:12:48PM +0100, Toke Høiland-Jørgensen wrote:
> The recent commit 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are
> taken from selftests dir") broke compilation against libbpf if it is installed
> on the system, and $INCLUDEDIR/bpf is not in the include path.
> 
> Since having the bpf/ subdir of $INCLUDEDIR in the include path has never been a
> requirement for building against libbpf before, this needs to be fixed. One
> option is to just revert the offending commit and figure out a different way to
> achieve what it aims for. However, this series takes a different approach:
> Changing all in-tree users of libbpf to consistently use a bpf/ prefix in
> #include directives for header files from libbpf.

I don't think such approach will work in all cases.
Consider the user installing libbpf headers into /home/somebody/include/bpf/,
passing that path to -I and trying to build bpf progs
that do #include "bpf_helpers.h"...
In the current shape of libbpf everything will compile fine,
but after patch 8 of this series the compiler will not find bpf/bpf_helper_defs.h.
So I think we have no choice, but to revert that part of Andrii's patch.
Note that doing #include "" for additional library headers is a common practice.
There was nothing wrong about #include "bpf_helper_defs.h" in bpf_helpers.h.
Toke Høiland-Jørgensen Jan. 15, 2020, 10:09 p.m. UTC | #3
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Wed, Jan 15, 2020 at 03:12:48PM +0100, Toke Høiland-Jørgensen wrote:
>> The recent commit 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are
>> taken from selftests dir") broke compilation against libbpf if it is installed
>> on the system, and $INCLUDEDIR/bpf is not in the include path.
>> 
>> Since having the bpf/ subdir of $INCLUDEDIR in the include path has never been a
>> requirement for building against libbpf before, this needs to be fixed. One
>> option is to just revert the offending commit and figure out a different way to
>> achieve what it aims for. However, this series takes a different approach:
>> Changing all in-tree users of libbpf to consistently use a bpf/ prefix in
>> #include directives for header files from libbpf.
>
> I don't think such approach will work in all cases.
> Consider the user installing libbpf headers into /home/somebody/include/bpf/,
> passing that path to -I and trying to build bpf progs
> that do #include "bpf_helpers.h"...
> In the current shape of libbpf everything will compile fine,
> but after patch 8 of this series the compiler will not find bpf/bpf_helper_defs.h.
> So I think we have no choice, but to revert that part of Andrii's patch.
> Note that doing #include "" for additional library headers is a common practice.
> There was nothing wrong about #include "bpf_helper_defs.h" in bpf_helpers.h.

OK, I'll take another look at that bit and see if I can get it to work
with #include "bpf_helper_defs.h" and still function with the read-only
tree (and avoid stale headers).

-Toke
Toke Høiland-Jørgensen Jan. 15, 2020, 10:10 p.m. UTC | #4
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Wed, Jan 15, 2020 at 6:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> The recent commit 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are
>> taken from selftests dir") broke compilation against libbpf if it is installed
>> on the system, and $INCLUDEDIR/bpf is not in the include path.
>>
>> Since having the bpf/ subdir of $INCLUDEDIR in the include path has never been a
>> requirement for building against libbpf before, this needs to be fixed. One
>> option is to just revert the offending commit and figure out a different way to
>> achieve what it aims for. However, this series takes a different approach:
>> Changing all in-tree users of libbpf to consistently use a bpf/ prefix in
>> #include directives for header files from libbpf.
>>
>> This turns out to be a somewhat invasive change in the number of files touched;
>> however, the actual changes to files are fairly trivial (most of them are simply
>> made with 'sed'). Also, this approach has the advantage that it makes external
>> and internal users consistent with each other, and ensures no future changes
>> breaks things in the same way as the commit referenced above.
>>
>> The series is split to make the change for one tool subdir at a time, while
>> trying not to break the build along the way. It is structured like this:
>>
>> - Patch 1-2: Trivial fixes to Makefiles for issues I discovered while changing
>>   the include paths.
>>
>> - Patch 3-7: Change the include directives to use the bpf/ prefix, and updates
>>   Makefiles to make sure tools/lib/ is part of the include path, but without
>>   removing tools/lib/bpf
>>
>> - Patch 8: Change the bpf_helpers file in libbpf itself to use the bpf/ prefix
>>   when including (the original source of breakage).
>>
>> - Patch 9-10: Remove tools/lib/bpf from include paths to make sure we don't
>>   inadvertently re-introduce includes without the bpf/ prefix.
>>
>> ---
>
> Thanks, Toke, for this clean up! I tested it locally for my set up:
> runqslower, bpftool, libbpf, and selftests all build fine, so it looks
> good. My only concern is with selftests/bpf Makefile, we shouldn't
> build anything outside of selftests/bpf. Let's fix that. Thanks!

Great, thanks for testing! I'll fix up your comments (and Alexei's) and
submit another version tomorrow.

-Toke