On Wed, Jan 27, 2021 at 6:33 PM Bruce Richardson <bruce.richard...@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. -- David Marchand
