From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id 20D9A1B28A for ; Thu, 21 Dec 2017 15:50:44 +0100 (CET) Received: by mail-wr0-f193.google.com with SMTP id v21so14672910wrc.0 for ; Thu, 21 Dec 2017 06:50:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=ZqZCzcXJfh3PffDVw4C5Szx9737Umr5lOghDkogzg4c=; b=WoYyd3tIt0p9P6UsE+6eJWCHqQ0fmf7FHNeNq2F5CusstLna01kDejJgQ0za5cLZp9 B3EfoNBBPezMPtl3Tup40FWXoCJJ9qSDeBBRyvfBCP3qvjT833AaNvqTpwT0f66I77SS HRBYJeC2nZujoO9r5xVhD4voYmQ4aFpNwwNQtLJbW+goIEp0fXagiwBAn3SKpX80MhnQ CsR9eJjBWLDZhtJbgR9r/vTe+GE2qv5aVLUEssw2myPI0QoM3FibMZT3wkmXenyMuCVl zcM+1qga9nyJOLHrHTn66gCHKNkp5r4xPmOC/zIldWRLkh3EzgPfZpmSZOWqDOOpAPMv OCSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=ZqZCzcXJfh3PffDVw4C5Szx9737Umr5lOghDkogzg4c=; b=LBhnBkgRnGzqbEgsv7QYrHTyY+EAGui6S0cYth+cFAk8dexwcOPywIeYH2MZbKDws1 gjluD93UF34ycYf6nlFL1Dq0oc2lef8ctcqEDu69ZDmHUL1O78GYhaHfYDyU7KJQZ3wc 5nvBpz05fuXgleXXlVwjE5CnePCNuAaYzCtnBx/nq8QlRsp4bq0L10JGeq7NR7oKZ4yN obGx1TnDxMvMAIFpnZKXEApvwvbXFxro00UVzpmtwZ7SCSMCBPBb5ve7499hLCuuYZ/s Nwn1HmEiGAIvksnHHhTh9ZmpybkE7v18uawxehuPkKSuTaHg6MLu/l2q8r6qKUYRfA6y TxXw== X-Gm-Message-State: AKGB3mJAe9kItYh/kkioljVljqfWLwdtkfLsNY6d8qa/cx0FhLBubcD5 m4naaKh8ym3epx732RwanZ5Jcg== X-Google-Smtp-Source: ACJfBosxubpGhYqPKNbew9Gs4Iiw0E4mTFxL6KbAriDBsgRrro+lmStGvJF7FXaeJ2RpweeAPZDLuA== X-Received: by 10.223.158.136 with SMTP id a8mr12188623wrf.47.1513867844553; Thu, 21 Dec 2017 06:50:44 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id a23sm6014358wra.70.2017.12.21.06.50.43 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Dec 2017 06:50:43 -0800 (PST) Date: Thu, 21 Dec 2017 15:50:31 +0100 From: Adrien Mazarguil To: Bruce Richardson Cc: dev@dpdk.org, Thomas Monjalon Message-ID: <20171221145031.GM5377@6wind.com> References: <20171221122458.811-1-adrien.mazarguil@6wind.com> <20171221122458.811-6-adrien.mazarguil@6wind.com> <20171221141257.GB8256@bricha3-MOBL3.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20171221141257.GB8256@bricha3-MOBL3.ger.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH v1 5/6] fix missing includes in exported headers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Dec 2017 14:50:45 -0000 On Thu, Dec 21, 2017 at 02:12:57PM +0000, Bruce Richardson wrote: > On Thu, Dec 21, 2017 at 02:00:04PM +0100, Adrien Mazarguil wrote: > > Many exported headers rely on definitions found in rte_config.h without > > including it, as shown by the following command: > > > > grep -L '^#include ' -- \ > > $(grep -Rl \ > > $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \ > > build/include/rte_config.h) \ > > -- build/include/) > > > > We cannot assume external applications will include rte_config.h on their > > own, neither directly nor through a -include parameter like DPDK does > > internally. > > > > This not only causes obvious compilation failures that can be reproduced > > with check-includes.sh such as: > > > > [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in > > this scope > > #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE) > > ^ > > > > It also results in less visible issues, for instance rte_hash_crc.h relying > > on RTE_ARCH_X86_64's presence to provide dedicated inline functions. > > > > This patch partially reverts the commit below and adds missing include > > lines to the remaining files. > > > > Fixes: f1a7a5c5f404 ("remove include of generated config header") > > Cc: Thomas Monjalon > > > > Signed-off-by: Adrien Mazarguil > > --- > > Hi Adrien, > > Just FYI, when we move to the new DPDK build system and pass the > necessary build meta-data to the application using pkg-config, this > should be a non-issue, as the pkg-config information will include the > "-include rte_config.h" parameter. Right, actually -include rte_config.h is also provided to applications that rely on the current DPDK build system, those are already safe. The motivation behind this patch is other applications that casually #include things without even depending on the build features of DPDK (not even going through pkg-config manually), in order to meet a safety level worthy of /usr/include. In my opinion it's fine if this usage is not supported and a prior #include is required, however in that case we need our header files to #error out. Adding #include directly as done in this patch was in fact just as easy. > When investigating that, I also tried this approach of adding rte_config > to files explicitly but it did not work for me as expected, as there > were cases where the build was depending upon the rte_config.h always > being the first include in the file. Normally, the rte_* headers should > be last included, so putting it at the top just didn't seem right to me. > I don't remember the specifics, but it was something like using the RTE_ > defines to determine which system header file to use e.g. BSD vs Linux. > However, this may be an internal DPDK-build restriction rather than one > that would affect user-apps our or examples. Was it perhaps before we added consistency to all public headers? check-includes.sh was added a few releases ago to make sure each of them could be included on its own, to ensure they properly fetched all their dependencies instead of triggering compilation errors. Rule of thumb being if you need something, #include the file providing it first. I find it strange rte_config.h is somehow an exception. > So, with transitioning to meson and pkg-config, this issue becomes less > significant. Agreed, however I still think something needs to be done, so: - Do we want to let applications not rely on our build flags yet still use our exported headers? - Is a missing #include supposed to trigger an explicit #error for safety? - How about putting back #include in our exported header files directly given the above? Note this discussion is only about exported headers. The rest remains unaffected and can safely continue to rely on -include rte_config.h. -- Adrien Mazarguil 6WIND