Re: [PATCH v2 05/13] backports: use BACKPORT_DIR prefix on kconfig sources

[Luis R. Rodriguez]

On Wed, Nov 05, 2014 at 08:51:45AM +0100, Johannes Berg wrote:
> On Tue, 2014-11-04 at 19:18 -0800, Luis R. Rodriguez wrote:
> 
> > @@ -21,23 +22,53 @@ class ConfigTree(object):
> >          yield f
> >          for l in open(os.path.join(self.basedir, f), 'r'):
> >              m = src_line.match(l)
> > -            if m and os.path.exists(os.path.join(self.basedir, m.group('src'))):
> > -                for i in self._walk(m.group('src')):
> > -                    yield i
> > +            if m:
> > +                bm = bk_src_line.match(l)
> > +                if bm:
> > +                    if os.path.exists(os.path.join(self.basedir, bm.group('src'))):
> > +                        for i in self._walk(os.path.join(self.basedir, bm.group('src'))):
> > +                            yield i
> > +                    elif os.path.exists(os.path.join(self.basedir, 'backports/' + bm.group('src'))):
> > +                        for i in self._walk(os.path.join(self.basedir, 'backports/' + bm.group('src'))):
> > +                            yield i
> > +                else:
> > +                    if os.path.exists(os.path.join(self.basedir, m.group('src'))):
> > +                        for i in self._walk(m.group('src')):
> > +                            yield i
> 
> Shouldn't this depend on "integrate" rather than on existence?
> 
> Ah, this is what you were alluding to in the commit log? The fact that
> you hardcode "backports/" into this? IMHO it would make more sense to
> pass in the "base" directory (either "backports/" or "") though, instead
> of making *that* depend on the existence of the directory as well.
> 
> Remember, the existence check here serves to remove includes that cannot
> be satisfied; your existence check mixes in the differentiation between
> packaged and integrated, which doesn't seem right.
> 
> Regardless of whether you make the "backports/" prefix configurable or
> not, you should pass it to this library function and use it
> unconditionally instead of trying to determine package vs. integrated
> from the existence of directories.

Fair enough.

> >      def _prune_sources(self, f, ignore):
> >          for nf in self._walk(f):
> >              out = ''
> >              for l in open(os.path.join(self.basedir, nf), 'r'):
> > -                m = src_line.match(l)
> > -                if not m:
> > -                    out += l
> > -                    continue
> > -                src = m.group('src')
> > -                if src in ignore or os.path.exists(os.path.join(self.basedir, src)):
> > -                    out += l
> > +                bm = bk_src_line.match(l)
> > +                if bm:
> > +                    bp_src = bm.group('src')
> > +                    if bp_src in ignore or \
> > +                       os.path.exists(os.path.join(self.basedir, bp_src)) or \
> > +                       os.path.exists(os.path.join(self.basedir, 'backports/' + bp_src)):
> 
> same here.

Yeah I get it, good points.

This does mean that bp_prefix topic *can* also be tied down
with this other directory prefix as a form of 'builder' for
integration. Making the prefix configurable would make sense
then only if also making the directory prefix should be
configurable.

I think we're better off right now with just supporting two
approaches with their own directory prefix, and prefixes
for variables.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/