From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7D6FCA04C8; Fri, 18 Sep 2020 13:43:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D655A1D955; Fri, 18 Sep 2020 13:43:39 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id A38E41D953 for ; Fri, 18 Sep 2020 13:43:38 +0200 (CEST) IronPort-SDR: 13zko6iKeOcglUKPY3w9RSTrbv3I5hT8R62KKymN0WAQbwol1s7ScTt8+s/IAAUdHvzKVnGnmV CrqSSl58Cr3A== X-IronPort-AV: E=McAfee;i="6000,8403,9747"; a="178006347" X-IronPort-AV: E=Sophos;i="5.77,274,1596524400"; d="scan'208";a="178006347" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Sep 2020 04:43:37 -0700 IronPort-SDR: kHFqKbRtZ5vTp93DDGbafdIi0eSwxZJNYQ1fsFHCuWxzjI1O8Nm7hTJlzBwNAqBL6pacSh9prm roO/HwGEmrow== X-IronPort-AV: E=Sophos;i="5.77,274,1596524400"; d="scan'208";a="484172995" Received: from bricha3-mobl.ger.corp.intel.com ([10.249.147.177]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 18 Sep 2020 04:43:35 -0700 Date: Fri, 18 Sep 2020 12:43:29 +0100 From: Bruce Richardson To: Mohammed Hawari Cc: dev@dpdk.org Message-ID: <20200918114329.GA1589@bricha3-MOBL.ger.corp.intel.com> References: <20200918084924.31784-1-mohammed@hawari.fr> <20200918084924.31784-2-mohammed@hawari.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200918084924.31784-2-mohammed@hawari.fr> Subject: Re: [dpdk-dev] [PATCH 1/1] build: allow disabling libs 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Sep 18, 2020 at 10:49:23AM +0200, Mohammed Hawari wrote: > Similarly to the disable_drivers option, the disable_libs option is > introduced. This allows to selectively disable the build of elements > in libs to speed-up the build process. > > Signed-off-by: Mohammed Hawari > --- While I don't particularly like allowing libs to be enabled and disabled since it complicates the build, I can see why it's necessary. This is an area that does need some discussion, as I believe others have some opinions in this area too. However, for now, some additional thoughts, both on this patch and in general: 1. I see you included disabling apps if their required libs are not available. What about the drivers though? 2. A bigger issue is whether this is really what we want to do, guarantee a passing build even if vast chunks of DPDK are actually enabled? I'd tend towards "no" in this case, and I'd rather see disabling of libs more constrained. 3. To this end, I think I'd rather see us maintain a set of libs which are allowed to be disabled, and prevent the rest from being so. For example, it makes no sense in DPDK to disable the EAL or mempool libs, since nothing will build, while the bitrate_stats or latency_stats libs could likely be disabled with little or no impact. Therefore, I think a better implementation is to start as in this patch with a new config parameter to disable libs, but as part of that patch add in an internal list of the libs which are allowed to be disabled (initially empty). Telling the build system to disable a lib not on that list should raise a configuration time error. As for how a lib gets on the list - that should be done once the build has been tested with that lib disabled, i.e. once testpmd and other apps have got #defines in the code for stripping out the disabled blocks, and any drivers which depend on the lib have proper checks and warnings in place about it being disabled (or also #defines in the code if that can be done). The other advantage of maintaining such a list is that it then becomes somewhat feasible to test these build settings, in that (maybe once per release), iterate through the list of disable-able libs and test that the build passed with each one disabled individually. [I think for this purpose we can ignore interactions of having two disabled simultaneously, in order to have something testable] What do others in the community think? Regards, /Bruce