Re: [dpdk-dev] [PATCH v6 0/8] add checking of header includes
From: David Marchand <hidden>
Date: 2021-01-28 10:55:55
On Wed, Jan 27, 2021 at 6:33 PM Bruce Richardson [off-list ref] 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. -- David Marchand