| Message ID | 20210127173330.1671341-1-bruce.richardson@intel.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 83D2CA052A; Wed, 27 Jan 2021 18:33:41 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1D0FB140FC0; Wed, 27 Jan 2021 18:33:41 +0100 (CET) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 71879140FB4 for <dev@dpdk.org>; Wed, 27 Jan 2021 18:33:38 +0100 (CET) IronPort-SDR: HDrhAf7ZfDeaMS9LKTNLsvRWDBn3oyrDK6qHoUbgSmUOzvQNWee5cIkdrcPj6VMI/sv1OOIL7r 4SaAt6Eq9MFg== X-IronPort-AV: E=McAfee;i="6000,8403,9877"; a="159277383" X-IronPort-AV: E=Sophos;i="5.79,380,1602572400"; d="scan'208";a="159277383" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jan 2021 09:33:37 -0800 IronPort-SDR: HTeDFqP4Rvc1YSUAPeQIv+EYNYXmva555NpDvNEdezGjR9nVfN5jpGjSdZr4S1wF4uOWgwTSXv S5M3BF1AQpxA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.79,380,1602572400"; d="scan'208";a="362506901" Received: from silpixa00399126.ir.intel.com ([10.237.222.4]) by fmsmga008.fm.intel.com with ESMTP; 27 Jan 2021 09:33:35 -0800 From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Cc: david.marchand@redhat.com, Bruce Richardson <bruce.richardson@intel.com> Date: Wed, 27 Jan 2021 17:33:22 +0000 Message-Id: <20210127173330.1671341-1-bruce.richardson@intel.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20210114110606.21142-1-bruce.richardson@intel.com> References: <20210114110606.21142-1-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-dev] [PATCH v6 0/8] add checking of header includes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
| Series |
add checking of header includes
|
|
Message
Bruce Richardson
Jan. 27, 2021, 5:33 p.m. UTC
As a general principle, each header file should include any other
headers it needs to provide data type definitions or macros. For
example, any header using the uintX_t types in structures or function
prototypes should include "stdint.h" to provide those type definitions.
In practice, while many, but not all, headers in DPDK did include all
necessary headers, it was never actually checked that each header could
be included in a C file and compiled without having any compiler errors
about missing definitions. The script "check-includes.sh" could be used
for this job, but it was not called out in the documentation, so many
contributors may not have been aware of it's existance. It also was
difficult to run from a source-code directory, as the script did not
automatically allow finding of headers from one DPDK library directory
to another [this was probably based on running it on a build created by
the "make" build system, where all headers were in a single directory].
To attempt to have a build-system integrated replacement, this patchset
adds a "chkincs" app in the buildtools directory to verify this on an
ongoing basis.
This chkincs app does nothing when run, and is not installed as part of
a DPDK "ninja install", it's for build-time checking only. Its source
code consists of one C file per public DPDK header, where that C file
contains nothing except an include for that header. Therefore, if any
header is added to the lib folder which fails to compile when included
alone, the build of chkincs will fail with a suitable error message.
Since this compile checking is not needed on most builds of DPDK, the
building of chkincs is disabled by default, but can be enabled by the
"test_includes" meson option. To catch errors with patch submissions,
the final patch of this series enables it for a single build in
test-meson-builds script.
Future work could involve doing similar checks on headers for C++
compatibility, which was something done by the check-includes.sh script
but which is missing here.
V6:
* Added release notes updates for:
- renamed, no-longer-installed header files
- new "check_includes" build option
- removal of old check_includes script
* Included acks from previous versions
V5:
* Removed private ethdev headers from checks and installing
* Removed the ALLOW_INTERNAL_APIS macro from chkincs build.
V4:
* Fixed build errors with clang and arm builds
* Added support for running chkincs as part of github actions CI
V3:
* Shrunk patchset as most header fixes already applied
* Moved chkincs from "apps" to the "buildtools" directory, which is a
better location for something not for installation for end-user use.
* Added patch to drop check-includes script.
V2:
* Add maintainers file entry for new app
* Drop patch for c11 ring header
* Use build variable "headers_no_chkincs" for tracking exceptions
Bruce Richardson (8):
eal: add missing include to mcslock
eal: fix error attribute use for clang
rib: fix missing header include
ethdev: make driver-only headers private
build: separate out headers for include checking
buildtools/chkincs: add app to verify header includes
devtools: remove check-includes script
ci: add checking of includes to CI builds
.ci/linux-build.sh | 1 +
MAINTAINERS | 5 +-
app/test/test_link_bonding.c | 2 +-
app/test/test_pdump.c | 2 +-
app/test/virtual_pmd.c | 2 +-
buildtools/chkincs/gen_c_file_for_header.py | 12 +
buildtools/chkincs/main.c | 4 +
buildtools/chkincs/meson.build | 40 +++
devtools/check-includes.sh | 259 ------------------
devtools/test-meson-builds.sh | 2 +-
doc/guides/contributing/coding_style.rst | 12 +
doc/guides/rel_notes/release_21_02.rst | 18 ++
drivers/bus/dpaa/dpaa_bus.c | 2 +-
drivers/bus/dpaa/include/fman.h | 2 +-
drivers/bus/fslmc/fslmc_bus.c | 2 +-
drivers/bus/fslmc/fslmc_vfio.c | 2 +-
drivers/bus/fslmc/portal/dpaa2_hw_dpbp.c | 2 +-
drivers/bus/fslmc/portal/dpaa2_hw_dpci.c | 2 +-
drivers/bus/fslmc/portal/dpaa2_hw_dpio.c | 2 +-
drivers/event/dpaa2/dpaa2_eventdev.c | 2 +-
drivers/event/dpaa2/dpaa2_hw_dpcon.c | 2 +-
drivers/event/octeontx/ssovf_evdev.c | 2 +-
drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 2 +-
drivers/net/af_packet/rte_eth_af_packet.c | 4 +-
drivers/net/af_xdp/rte_eth_af_xdp.c | 4 +-
drivers/net/ark/ark_ethdev.c | 2 +-
drivers/net/ark/ark_ethdev_rx.h | 2 +-
drivers/net/ark/ark_ethdev_tx.h | 2 +-
drivers/net/ark/ark_ext.h | 2 +-
drivers/net/ark/ark_global.h | 2 +-
drivers/net/ark/ark_pktchkr.c | 2 +-
drivers/net/ark/ark_pktgen.c | 2 +-
drivers/net/atlantic/atl_ethdev.c | 2 +-
drivers/net/atlantic/atl_rxtx.c | 2 +-
drivers/net/atlantic/rte_pmd_atlantic.c | 2 +-
drivers/net/avp/avp_ethdev.c | 4 +-
drivers/net/axgbe/axgbe_common.h | 2 +-
drivers/net/bnx2x/bnx2x_ethdev.c | 2 +-
drivers/net/bnx2x/bnx2x_ethdev.h | 2 +-
drivers/net/bnxt/bnxt.h | 2 +-
drivers/net/bnxt/bnxt_ethdev.c | 4 +-
drivers/net/bnxt/bnxt_stats.h | 2 +-
drivers/net/bnxt/rte_pmd_bnxt.c | 2 +-
drivers/net/bonding/eth_bond_private.h | 2 +-
drivers/net/bonding/rte_eth_bond_api.c | 2 +-
drivers/net/bonding/rte_eth_bond_pmd.c | 4 +-
drivers/net/cxgbe/base/t4_hw.c | 2 +-
drivers/net/cxgbe/base/t4vf_hw.c | 2 +-
drivers/net/cxgbe/cxgbe_ethdev.c | 4 +-
drivers/net/cxgbe/cxgbe_main.c | 4 +-
drivers/net/cxgbe/cxgbevf_ethdev.c | 4 +-
drivers/net/cxgbe/cxgbevf_main.c | 4 +-
drivers/net/cxgbe/sge.c | 2 +-
drivers/net/dpaa/dpaa_ethdev.c | 2 +-
drivers/net/dpaa/dpaa_ethdev.h | 2 +-
drivers/net/dpaa/dpaa_rxtx.c | 2 +-
drivers/net/dpaa2/base/dpaa2_hw_dpni.c | 2 +-
drivers/net/dpaa2/dpaa2_ethdev.c | 2 +-
drivers/net/dpaa2/dpaa2_rxtx.c | 2 +-
drivers/net/e1000/em_ethdev.c | 4 +-
drivers/net/e1000/em_rxtx.c | 2 +-
drivers/net/e1000/igb_ethdev.c | 4 +-
drivers/net/e1000/igb_flow.c | 4 +-
drivers/net/e1000/igb_pf.c | 2 +-
drivers/net/e1000/igb_rxtx.c | 2 +-
drivers/net/ena/ena_ethdev.c | 4 +-
drivers/net/enetc/enetc_ethdev.c | 2 +-
drivers/net/enic/enic_ethdev.c | 4 +-
drivers/net/enic/enic_flow.c | 2 +-
drivers/net/enic/enic_fm_flow.c | 2 +-
drivers/net/enic/enic_main.c | 2 +-
drivers/net/enic/enic_res.c | 2 +-
drivers/net/enic/enic_rxtx.c | 2 +-
drivers/net/enic/enic_rxtx_vec_avx2.c | 2 +-
drivers/net/enic/enic_vf_representor.c | 4 +-
drivers/net/failsafe/failsafe.c | 4 +-
drivers/net/failsafe/failsafe_ops.c | 2 +-
drivers/net/failsafe/failsafe_private.h | 2 +-
drivers/net/failsafe/failsafe_rxtx.c | 2 +-
drivers/net/fm10k/fm10k_ethdev.c | 4 +-
drivers/net/fm10k/fm10k_rxtx.c | 2 +-
drivers/net/fm10k/fm10k_rxtx_vec.c | 2 +-
drivers/net/hinic/base/hinic_pmd_hwdev.c | 2 +-
drivers/net/hinic/hinic_pmd_ethdev.c | 2 +-
drivers/net/hinic/hinic_pmd_ethdev.h | 2 +-
drivers/net/hns3/hns3_cmd.c | 2 +-
drivers/net/hns3/hns3_ethdev.c | 2 +-
drivers/net/hns3/hns3_ethdev.h | 2 +-
drivers/net/hns3/hns3_ethdev_vf.c | 2 +-
drivers/net/hns3/hns3_fdir.c | 2 +-
drivers/net/hns3/hns3_mbx.c | 2 +-
drivers/net/hns3/hns3_mp.c | 2 +-
drivers/net/hns3/hns3_regs.c | 2 +-
drivers/net/hns3/hns3_rxtx.c | 2 +-
drivers/net/hns3/hns3_rxtx_vec.c | 2 +-
drivers/net/hns3/hns3_rxtx_vec_sve.c | 2 +-
drivers/net/i40e/i40e_ethdev.c | 4 +-
drivers/net/i40e/i40e_ethdev_vf.c | 4 +-
drivers/net/i40e/i40e_fdir.c | 2 +-
drivers/net/i40e/i40e_flow.c | 2 +-
drivers/net/i40e/i40e_pf.c | 2 +-
drivers/net/i40e/i40e_rxtx.c | 2 +-
drivers/net/i40e/i40e_rxtx_vec_altivec.c | 2 +-
drivers/net/i40e/i40e_rxtx_vec_avx2.c | 2 +-
drivers/net/i40e/i40e_rxtx_vec_avx512.c | 2 +-
drivers/net/i40e/i40e_rxtx_vec_common.h | 2 +-
drivers/net/i40e/i40e_rxtx_vec_neon.c | 2 +-
drivers/net/i40e/i40e_rxtx_vec_sse.c | 2 +-
drivers/net/i40e/i40e_vf_representor.c | 2 +-
drivers/net/iavf/iavf_ethdev.c | 4 +-
drivers/net/iavf/iavf_fdir.c | 2 +-
drivers/net/iavf/iavf_generic_flow.c | 2 +-
drivers/net/iavf/iavf_hash.c | 2 +-
drivers/net/iavf/iavf_rxtx.c | 2 +-
drivers/net/iavf/iavf_rxtx_vec_common.h | 2 +-
drivers/net/iavf/iavf_rxtx_vec_sse.c | 2 +-
drivers/net/iavf/iavf_vchnl.c | 4 +-
drivers/net/ice/ice_acl_filter.c | 2 +-
drivers/net/ice/ice_dcf.c | 4 +-
drivers/net/ice/ice_dcf.h | 2 +-
drivers/net/ice/ice_dcf_ethdev.c | 2 +-
drivers/net/ice/ice_ethdev.c | 2 +-
drivers/net/ice/ice_ethdev.h | 2 +-
drivers/net/ice/ice_generic_flow.c | 2 +-
drivers/net/ice/ice_hash.c | 2 +-
drivers/net/ice/ice_rxtx.c | 2 +-
drivers/net/ice/ice_switch_filter.c | 2 +-
drivers/net/igc/igc_ethdev.c | 4 +-
drivers/net/igc/igc_filter.h | 2 +-
drivers/net/igc/igc_txrx.c | 2 +-
drivers/net/ionic/ionic_ethdev.c | 4 +-
drivers/net/ionic/ionic_lif.c | 2 +-
drivers/net/ionic/ionic_rxtx.c | 2 +-
drivers/net/ipn3ke/ipn3ke_ethdev.c | 2 +-
drivers/net/ipn3ke/ipn3ke_ethdev.h | 4 +-
drivers/net/ipn3ke/ipn3ke_flow.c | 2 +-
drivers/net/ipn3ke/ipn3ke_representor.c | 2 +-
drivers/net/ipn3ke/ipn3ke_tm.c | 2 +-
drivers/net/ixgbe/ixgbe_bypass.c | 2 +-
drivers/net/ixgbe/ixgbe_ethdev.c | 4 +-
drivers/net/ixgbe/ixgbe_fdir.c | 2 +-
drivers/net/ixgbe/ixgbe_flow.c | 2 +-
drivers/net/ixgbe/ixgbe_ipsec.c | 4 +-
drivers/net/ixgbe/ixgbe_pf.c | 2 +-
drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 2 +-
drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c | 2 +-
drivers/net/ixgbe/ixgbe_rxtx_vec_sse.c | 2 +-
drivers/net/ixgbe/ixgbe_vf_representor.c | 2 +-
drivers/net/ixgbe/rte_pmd_ixgbe.c | 2 +-
drivers/net/kni/rte_eth_kni.c | 4 +-
drivers/net/liquidio/base/lio_23xx_vf.c | 2 +-
drivers/net/liquidio/base/lio_mbox.c | 2 +-
drivers/net/liquidio/lio_ethdev.c | 4 +-
drivers/net/liquidio/lio_rxtx.c | 2 +-
drivers/net/memif/memif_socket.c | 4 +-
drivers/net/memif/rte_eth_memif.c | 4 +-
drivers/net/memif/rte_eth_memif.h | 2 +-
drivers/net/mlx4/mlx4.c | 4 +-
drivers/net/mlx4/mlx4.h | 2 +-
drivers/net/mlx4/mlx4_ethdev.c | 2 +-
drivers/net/mlx4/mlx4_flow.c | 2 +-
drivers/net/mlx4/mlx4_flow.h | 2 +-
drivers/net/mlx4/mlx4_intr.c | 2 +-
drivers/net/mlx4/mlx4_mp.c | 2 +-
drivers/net/mlx4/mlx4_rxq.c | 2 +-
drivers/net/mlx4/mlx4_rxtx.h | 2 +-
drivers/net/mlx4/mlx4_txq.c | 2 +-
drivers/net/mlx5/linux/mlx5_ethdev_os.c | 2 +-
drivers/net/mlx5/linux/mlx5_mp_os.c | 2 +-
drivers/net/mlx5/linux/mlx5_os.c | 4 +-
drivers/net/mlx5/linux/mlx5_verbs.c | 2 +-
drivers/net/mlx5/linux/mlx5_vlan_os.c | 2 +-
drivers/net/mlx5/mlx5.c | 4 +-
drivers/net/mlx5/mlx5.h | 2 +-
drivers/net/mlx5/mlx5_defs.h | 2 +-
drivers/net/mlx5/mlx5_ethdev.c | 2 +-
drivers/net/mlx5/mlx5_flow.c | 2 +-
drivers/net/mlx5/mlx5_flow_dv.c | 2 +-
drivers/net/mlx5/mlx5_flow_verbs.c | 2 +-
drivers/net/mlx5/mlx5_mac.c | 2 +-
drivers/net/mlx5/mlx5_rss.c | 2 +-
drivers/net/mlx5/mlx5_rxmode.c | 2 +-
drivers/net/mlx5/mlx5_rxq.c | 2 +-
drivers/net/mlx5/mlx5_stats.c | 2 +-
drivers/net/mlx5/mlx5_trigger.c | 2 +-
drivers/net/mlx5/mlx5_txpp.c | 2 +-
drivers/net/mlx5/mlx5_txq.c | 2 +-
drivers/net/mlx5/mlx5_vlan.c | 2 +-
drivers/net/mlx5/windows/mlx5_ethdev_os.c | 2 +-
drivers/net/mlx5/windows/mlx5_mp_os.c | 2 +-
drivers/net/mlx5/windows/mlx5_os.c | 2 +-
drivers/net/mlx5/windows/mlx5_vlan_os.c | 2 +-
drivers/net/mvneta/mvneta_ethdev.c | 2 +-
drivers/net/mvpp2/mrvl_ethdev.c | 2 +-
drivers/net/netvsc/hn_ethdev.c | 2 +-
drivers/net/netvsc/hn_rndis.c | 2 +-
drivers/net/netvsc/hn_vf.c | 2 +-
drivers/net/nfb/nfb_ethdev.c | 2 +-
drivers/net/nfb/nfb_tx.h | 2 +-
drivers/net/nfp/nfp_net.c | 4 +-
drivers/net/nfp/nfpcore/nfp_cpp.h | 2 +-
drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 2 +-
drivers/net/nfp/nfpcore/nfp_cppcore.c | 2 +-
drivers/net/null/rte_eth_null.c | 4 +-
drivers/net/octeontx/octeontx_ethdev.h | 2 +-
drivers/net/octeontx/octeontx_rxtx.c | 2 +-
drivers/net/octeontx/octeontx_rxtx.h | 2 +-
drivers/net/octeontx2/otx2_ethdev.c | 2 +-
drivers/net/octeontx2/otx2_link.c | 2 +-
drivers/net/octeontx2/otx2_ptp.c | 2 +-
drivers/net/pcap/rte_eth_pcap.c | 4 +-
drivers/net/pfe/pfe_eth.h | 2 +-
drivers/net/pfe/pfe_ethdev.c | 2 +-
drivers/net/qede/qede_ethdev.h | 4 +-
drivers/net/ring/rte_eth_ring.c | 2 +-
drivers/net/sfc/sfc.h | 2 +-
drivers/net/sfc/sfc_dp_rx.h | 2 +-
drivers/net/sfc/sfc_dp_tx.h | 2 +-
drivers/net/sfc/sfc_ethdev.c | 4 +-
drivers/net/sfc/sfc_ev.h | 2 +-
drivers/net/sfc/sfc_flow.c | 2 +-
drivers/net/sfc/sfc_rx.h | 2 +-
drivers/net/sfc/sfc_tx.h | 2 +-
drivers/net/softnic/rte_eth_softnic.c | 4 +-
.../net/softnic/rte_eth_softnic_internals.h | 2 +-
drivers/net/szedata2/rte_eth_szedata2.c | 4 +-
drivers/net/tap/rte_eth_tap.c | 4 +-
drivers/net/tap/rte_eth_tap.h | 2 +-
drivers/net/thunderx/nicvf_ethdev.c | 4 +-
drivers/net/thunderx/nicvf_ethdev.h | 2 +-
drivers/net/thunderx/nicvf_rxtx.c | 2 +-
drivers/net/thunderx/nicvf_rxtx.h | 2 +-
drivers/net/thunderx/nicvf_struct.h | 2 +-
drivers/net/txgbe/txgbe_ethdev.c | 2 +-
drivers/net/txgbe/txgbe_ipsec.c | 2 +-
drivers/net/txgbe/txgbe_pf.c | 2 +-
drivers/net/txgbe/txgbe_rxtx.c | 2 +-
drivers/net/vhost/rte_eth_vhost.c | 4 +-
drivers/net/virtio/virtio_ethdev.c | 4 +-
drivers/net/virtio/virtio_pci.h | 2 +-
drivers/net/virtio/virtio_rxtx.c | 2 +-
drivers/net/virtio/virtio_rxtx_simple.c | 2 +-
.../net/virtio/virtio_rxtx_simple_altivec.c | 2 +-
drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 +-
drivers/net/virtio/virtio_rxtx_simple_sse.c | 2 +-
drivers/net/virtio/virtio_user_ethdev.c | 2 +-
drivers/net/vmxnet3/vmxnet3_ethdev.c | 4 +-
drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
lib/librte_eal/include/generic/rte_mcslock.h | 1 +
lib/librte_eal/include/meson.build | 2 +-
lib/librte_eal/include/rte_compat.h | 8 +-
lib/librte_eal/x86/include/meson.build | 14 +-
.../{rte_ethdev_driver.h => ethdev_driver.h} | 0
.../{rte_ethdev_pci.h => ethdev_pci.h} | 2 +-
lib/librte_ethdev/ethdev_private.c | 2 +-
.../{rte_ethdev_vdev.h => ethdev_vdev.h} | 2 +-
lib/librte_ethdev/meson.build | 8 +-
lib/librte_ethdev/rte_class_eth.c | 2 +-
lib/librte_ethdev/rte_ethdev.c | 2 +-
lib/librte_ethdev/rte_ethdev.h | 2 +-
lib/librte_ethdev/rte_flow_driver.h | 2 +-
lib/librte_ethdev/rte_mtr_driver.h | 2 +-
lib/librte_ethdev/rte_tm_driver.h | 2 +-
lib/librte_hash/meson.build | 4 +-
lib/librte_ipsec/meson.build | 3 +-
lib/librte_lpm/meson.build | 2 +-
lib/librte_regexdev/meson.build | 2 +-
lib/librte_rib/rte_rib6.h | 1 +
lib/librte_ring/meson.build | 4 +-
lib/librte_stack/meson.build | 4 +-
lib/librte_table/meson.build | 7 +-
lib/meson.build | 3 +
meson.build | 6 +
meson_options.txt | 2 +
275 files changed, 435 insertions(+), 579 deletions(-)
create mode 100755 buildtools/chkincs/gen_c_file_for_header.py
create mode 100644 buildtools/chkincs/main.c
create mode 100644 buildtools/chkincs/meson.build
delete mode 100755 devtools/check-includes.sh
rename lib/librte_ethdev/{rte_ethdev_driver.h => ethdev_driver.h} (100%)
rename lib/librte_ethdev/{rte_ethdev_pci.h => ethdev_pci.h} (99%)
rename lib/librte_ethdev/{rte_ethdev_vdev.h => ethdev_vdev.h} (97%)
--
2.27.0
Comments
On Wed, Jan 27, 2021 at 6:33 PM Bruce Richardson <bruce.richardson@intel.com> wrote: > > As a general principle, each header file should include any other > headers it needs to provide data type definitions or macros. For > example, any header using the uintX_t types in structures or function > prototypes should include "stdint.h" to provide those type definitions. > > In practice, while many, but not all, headers in DPDK did include all > necessary headers, it was never actually checked that each header could > be included in a C file and compiled without having any compiler errors > about missing definitions. The script "check-includes.sh" could be used > for this job, but it was not called out in the documentation, so many > contributors may not have been aware of it's existance. It also was > difficult to run from a source-code directory, as the script did not > automatically allow finding of headers from one DPDK library directory > to another [this was probably based on running it on a build created by > the "make" build system, where all headers were in a single directory]. > To attempt to have a build-system integrated replacement, this patchset > adds a "chkincs" app in the buildtools directory to verify this on an > ongoing basis. > > This chkincs app does nothing when run, and is not installed as part of > a DPDK "ninja install", it's for build-time checking only. Its source > code consists of one C file per public DPDK header, where that C file > contains nothing except an include for that header. Therefore, if any > header is added to the lib folder which fails to compile when included > alone, the build of chkincs will fail with a suitable error message. > Since this compile checking is not needed on most builds of DPDK, the > building of chkincs is disabled by default, but can be enabled by the > "test_includes" meson option. To catch errors with patch submissions, > the final patch of this series enables it for a single build in > test-meson-builds script. > > Future work could involve doing similar checks on headers for C++ > compatibility, which was something done by the check-includes.sh script > but which is missing here. > > V6: > * Added release notes updates for: > - renamed, no-longer-installed header files > - new "check_includes" build option > - removal of old check_includes script > * Included acks from previous versions I have some comments, see replies on patches. I can address them if you are ok, and I would take this series for -rc2 without needing a respin. Sidenote: I like how we are hiding API by simply not exporting headers. We need more cleanups like this. Ethdev has been cleaned; this will probably remove the need for the ABI exception on eth_dev_ops. Eventdev, other driver classes and bus drivers will probably be the next to look at.
On Thu, Jan 28, 2021 at 11:55:34AM +0100, David Marchand wrote: > On Wed, Jan 27, 2021 at 6:33 PM Bruce Richardson > <bruce.richardson@intel.com> wrote: > > > > As a general principle, each header file should include any other > > headers it needs to provide data type definitions or macros. For > > example, any header using the uintX_t types in structures or function > > prototypes should include "stdint.h" to provide those type definitions. > > > > In practice, while many, but not all, headers in DPDK did include all > > necessary headers, it was never actually checked that each header could > > be included in a C file and compiled without having any compiler errors > > about missing definitions. The script "check-includes.sh" could be used > > for this job, but it was not called out in the documentation, so many > > contributors may not have been aware of it's existance. It also was > > difficult to run from a source-code directory, as the script did not > > automatically allow finding of headers from one DPDK library directory > > to another [this was probably based on running it on a build created by > > the "make" build system, where all headers were in a single directory]. > > To attempt to have a build-system integrated replacement, this patchset > > adds a "chkincs" app in the buildtools directory to verify this on an > > ongoing basis. > > > > This chkincs app does nothing when run, and is not installed as part of > > a DPDK "ninja install", it's for build-time checking only. Its source > > code consists of one C file per public DPDK header, where that C file > > contains nothing except an include for that header. Therefore, if any > > header is added to the lib folder which fails to compile when included > > alone, the build of chkincs will fail with a suitable error message. > > Since this compile checking is not needed on most builds of DPDK, the > > building of chkincs is disabled by default, but can be enabled by the > > "test_includes" meson option. To catch errors with patch submissions, > > the final patch of this series enables it for a single build in > > test-meson-builds script. > > > > Future work could involve doing similar checks on headers for C++ > > compatibility, which was something done by the check-includes.sh script > > but which is missing here. > > > > V6: > > * Added release notes updates for: > > - renamed, no-longer-installed header files > > - new "check_includes" build option > > - removal of old check_includes script > > * Included acks from previous versions > > I have some comments, see replies on patches. > I can address them if you are ok, and I would take this series for > -rc2 without needing a respin. > Thomas has some feedback too now, and I think there are one or two patches where we might want to wait for consensus. However, if you are happy to take these as they are and do any fixups yourself feel free. > Sidenote: I like how we are hiding API by simply not exporting headers. > We need more cleanups like this. > Ethdev has been cleaned; this will probably remove the need for the > ABI exception on eth_dev_ops. > Eventdev, other driver classes and bus drivers will probably be the > next to look at. > Yes, there is plenty more cleanup work still needed. * The chkincs support added here integrates nicely into the build but does not fully support everything that the old script did, so investigation is needed especially for c++ checking support. * Beyond that we should also look to do cleanup based on IWYU to remove excess headers. This work gives us a nice safety-net for that as it should flag to us if we every remove too much from a public header.