Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754457AbaKEJW4 (ORCPT ); Wed, 5 Nov 2014 04:22:56 -0500 Received: from s3.sipsolutions.net ([5.9.151.49]:33744 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753936AbaKEJWw (ORCPT ); Wed, 5 Nov 2014 04:22:52 -0500 Message-ID: <1415179364.2589.13.camel@sipsolutions.net> Subject: Re: [PATCH v2 03/13] backports: allow for different backport prefix From: Johannes Berg To: "Luis R. Rodriguez" Cc: "Luis R. Rodriguez" , backports@vger.kernel.org, linux-kernel@vger.kernel.org, yann.morin.1998@free.fr, mmarek@suse.cz, sassmann@kpanic.de Date: Wed, 05 Nov 2014 10:22:44 +0100 In-Reply-To: <20141105091610.GM12953@wotan.suse.de> References: <1415157517-15442-1-git-send-email-mcgrof@do-not-panic.com> <1415157517-15442-4-git-send-email-mcgrof@do-not-panic.com> <1415173596.2589.3.camel@sipsolutions.net> <20141105091610.GM12953@wotan.suse.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.7-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2014-11-05 at 10:16 +0100, Luis R. Rodriguez wrote: > > IMHO this would be better handled in the code that uses the return value > > to add things to the Kconfig dependencies, there you could just go > > if integrate: > > deplist[sym] = ["BACKPORT_" + x for x in new] > > else: > > deplist[sym] = new > > I like it, thanks. > > I will note how you still provided "BACKPORT_" here rather than the prefix, > that's why I did the sub thing, but I'm more inclined to remove the dynamic > nature of the prefix for integration. Not sure why that'd be a good idea > and could only make things harder to support / change in the future as > we are learning with CPTCFG_. > > Thoughts? I think the splitting of bp_kconf_prefix and bp_prefix I suggested would help. Now that I look at it again, the names don't really make sense though. > > > @@ -838,7 +863,7 @@ def process(kerneldir, outdir, copy_list_file, git_revision=None, > > > for f in files: > > > data = open(os.path.join(root, f), 'r').read() > > > for r in regexes: > > > - data = r.sub(r'CPTCFG_\1', data) > > > + data = r.sub(r'' + bp_prefix + '\\1', data) > > > > technically, that should be re.escape(bp_prefix) > > We want to support bp_prefix having a regexp ? Sorry I didn't get that. No, I mean if bp_prefix were to contain some special character like [. This can't actually happen though. > > (btw, it might be clearer if you used %s instead of +'ing the bp_prefix > > in) > > Wasn't quite sure how'd well that'd look with the r'' prefix thing, and > still not sure, r.sub(r"%s\\1" % bp_prefix, data) ? If so that looks rather > like hell to me. Ok sure :) + is fine with me. > > > + def _mod_kconfig_line(self, l, orig_symbols, bp_prefix): > > > + if bp_prefix != 'CPTCFG_': > > > + prefix = re.sub(r'^CONFIG_(.*)', r'\1', bp_prefix) > > > > Another case like above ... maybe you should have bp_prefix and > > bp_kconf_prefix separately. Actually that seems like a good idea. > > bp_kconf_prefix is empty for the backport package case, so you could add > > it in unconditionally, and bp_prefix would be CONFIG_ > > You mean CONFIG_BACKPORT_ ? No, I did mean CONFIG_. One prefix for kconfig, and one prefix for "additional renaming". The names I gave them here were really bad though. Maybe bp_prefix = "CONFIG_" additional_prefix = "BACKPORT_" (or bp_prefix = "CPTCFG_" / additional_prefix = "") or so? > > or CPTCFG_ for the > > two cases. Yes, I think that would make a lot of sense and allow you to > > get rid of this regular expression magic while making the code easier to > > read/understand. > > Once this is clear sure, I do prefer it but only once we evaluate if we > really need to make the prefixes configurable. Yeah I guess we don't really, but I'd also hate to hardcode BACKPORT_ everywhere? > > > + def adjust_backported_configs(self, integrate, orig_symbols, bp_prefix): > > > > This is only used for integrated though, no? > > Right now yes, but I'm hinting that perhaps it should also be used for > packaging since it deals with negating a symbol if its built-in on > the kernel already. There should be other ways to do this for packaging, > the checks.h does it but that's just for two modules, we should be doing > this for much other symbols as well. Yeah that might be worthwhile. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/