Re: [PATCH v2 03/13] backports: allow for different backport prefix

[Johannes Berg <johannes@sipsolutions.net>]

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