From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 464E4A0A04; Fri, 15 Jan 2021 15:55:51 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 06F6C141125; Fri, 15 Jan 2021 15:55:51 +0100 (CET) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id 9AC28141124 for ; Fri, 15 Jan 2021 15:55:49 +0100 (CET) IronPort-SDR: e3f/o8nToaLzcbOhGi4ScjnonaEgXT02SBqrwxSRI2dI3E5OSqwiBmHfmcE9QG9iFaCBTh2riK SNOyOI7yPRXQ== X-IronPort-AV: E=McAfee;i="6000,8403,9864"; a="263351555" X-IronPort-AV: E=Sophos;i="5.79,349,1602572400"; d="scan'208";a="263351555" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Jan 2021 06:55:48 -0800 IronPort-SDR: 4Ip8+jw2Qjn5Z72Twskyt9iBOPUya7HN2n6541qJ4Y9FK2Ln7aJwZYfJ/Cc4CxSbxGYwKoNZ4m WTjYEkShRAvQ== X-IronPort-AV: E=Sophos;i="5.79,349,1602572400"; d="scan'208";a="354325250" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.8.25]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 15 Jan 2021 06:55:44 -0800 Date: Fri, 15 Jan 2021 14:55:41 +0000 From: Bruce Richardson To: Thomas Monjalon Cc: Ferruh Yigit , dev@dpdk.org, david.marchand@redhat.com, Andrew Rybchenko , Yipeng Wang , Sameh Gobriel , Konstantin Ananyev , Bernard Iremonger , Vladimir Medvedkin , Ori Kam , Honnappa Nagarahalli , Olivier Matz , Cristian Dumitrescu , adrien.mazarguil@6wind.com Message-ID: <20210115145541.GC1487@bricha3-MOBL.ger.corp.intel.com> References: <20210114110606.21142-1-bruce.richardson@intel.com> <9b696a65-2012-7ae8-d840-1515f8bf140b@intel.com> <20210115115905.GB1487@bricha3-MOBL.ger.corp.intel.com> <4629518.ttYPrCFMyY@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4629518.ttYPrCFMyY@thomas> Subject: Re: [dpdk-dev] [PATCH v2 16/19] app/chkincs: add chkincs app to verify headers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Jan 15, 2021 at 03:09:25PM +0100, Thomas Monjalon wrote: > 15/01/2021 12:59, Bruce Richardson: > > On Fri, Jan 15, 2021 at 11:51:49AM +0000, Ferruh Yigit wrote: > > > On 1/15/2021 11:10 AM, Bruce Richardson wrote: > > > > 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 > > > > --- > > > > > > > > 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. /Bruce