On Fri, Jan 15, 2021 at 02:59:08PM +0000, Bruce Richardson wrote:

On Fri, Jan 15, 2021 at 02:55:41PM +0000, Bruce Richardson wrote:

quoted

On Fri, Jan 15, 2021 at 03:09:25PM +0100, Thomas Monjalon wrote:

quoted

15/01/2021 12:59, Bruce Richardson:

quoted

On Fri, Jan 15, 2021 at 11:51:49AM +0000, Ferruh Yigit wrote:

quoted

On 1/15/2021 11:10 AM, Bruce Richardson wrote:

quoted

To verify that all DPDK headers are ok for inclusion directly in a C
file, and are not missing any other pre-requisite headers, we can
auto-generate for each header an empty C file that includes that header.
Compiling these files will throw errors if any header has unmet
dependencies.

The list of headers to check is based of the "headers" value returned from
each library's meson.build file. However, since not all headers are for
direct inclusion, add a build variable "headers_no_chkincs" to list those
headers and skip checking them.

Signed-off-by: Bruce Richardson <redacted>
---

v2:
* add maintainers entry
* distribute exception list among meson.build files.

  MAINTAINERS                              |  4 ++++
  app/chkincs/gen_c_file_for_header.py     | 12 ++++++++++
  app/chkincs/main.c                       |  4 ++++
  app/chkincs/meson.build                  | 28 ++++++++++++++++++++++++

+1 to have this kind of tool to check, but it is not an application like
others in the 'app' folder, what do you think placing it under 'devtools' or
'buildtools'?

Couple of reasons why it's placed in app.

1. We previously had a "chkincs" app in DPDK which was kept in the app
folder
2. It allows us to reuse the build infrastructure for building apps, rather
than reduplicating it.
3. We don't have any compilable code currently in the devtools folder, and
even in buildtools the pmdinfogen app is going to go away.

That being said, none of those reasons are major issues that can't be
worked around if the consensus is to move it.

It could be easily in devtools if it was a script.
By the way, we already have devtools/check-includes.sh
If your solution is better, please remove this script.

I only discovered the script existed when doing the v2 of this patchset,
since it showed up in some grep calls I did for exception cases. I'm not
sure that either approach is necessarily better, it's just right now that
the script is unused (and also unknown) which is why I did this cleanup
work.

Here is how I see the current comparison between two approaches:
* Script as advantage in that it performs C++ checks as well as C
* Script also allows passing arbitrary additional C flags into checks for
  higher levels of compliance, but I'm not sure this is something I like as
  I'd rather have standardisation here across all headers than have some
  headers more pedantic-friendly than others.
* Main downside of the script is that is works off directories rather than
  a list of files, which means it requires maintenance of the exception
  list in the script, rather than in the build definition files where we call
  out the headers to be installed

I'm honestly fine either way on this (as with directory where
implementation lives) - main thing is to have the checking done, rather
than ignored.

And I (obviously) forgot to mention that the existing script is not currently
integrated into existing build or build-test scripts. I haven't looked into
how complex this would be, but it would require investigation time.

I do plan to look into re-using the script and other options around this
area - hopefully in RC2 timeframe. However, to make the patchset rework a
little easier, perhaps the small header fixes could be picked up for ahead
of any rework?

/Bruce