[PULL] [L/unstable] merge configs into annotations
Andrea Righi
andrea.righi at canonical.com
Mon Dec 5 08:10:08 UTC 2022
Hey Masahiro,
thank you so much for your detailed review! Comments below.
On Mon, Dec 05, 2022 at 09:06:41AM +0900, Masahiro Yamada wrote:
> On Thu, Nov 17, 2022 at 3:09 AM Andrea Righi <andrea.righi at canonical.com> wrote:
>
>
>
> Here are more detailed comments for the annotation script.
>
>
> >
> > ----------------------------------------------------------------
> > Andrea Righi (13):
> > UBUNTU: [Packaging] config-check: ignore values that are not defined in annotations
> > UBUNTU: [Packaging] config-check: do not strictly enforce CONFIG_CC_VERSION_TEXT
> > UBUNTU: [Packaging] introduce annotations script
> > UBUNTU: [Packaging] automatically generate configs from annotations
> > UBUNTU: [Packaging] drop deprecated script splitconfig.pl
> > UBUNTU: [Packaging] drop deprecated script tristate.sh
> > UBUNTU: [Packaging] drop config-check and use annotations
> > UBUNTU: [Packaging] simplify kernelconfig
> > UBUNTU: [Packaging] kernelconfig: always keep configs
> > UBUNTU: [Packaging] provide listnewconfigs
> > UBUNTU: [Packaging] kernelconfig: introduce importconfigs
> > UBUNTU: [Config] import all configs into annotations
> > UBUNTU: [Config] drop configs and rely on annotations
> >
> > debian.master/config/amd64/config.common.amd64 | 701 -
> > debian.master/config/amd64/config.flavour.generic | 3 -
> > debian.master/config/annotations | 6808 +++++-----
> > debian.master/config/arm64/config.common.arm64 | 731 -
> > debian.master/config/arm64/config.flavour.generic | 14 -
> > .../config/arm64/config.flavour.generic-64k | 14 -
> > debian.master/config/armhf/config.common.armhf | 712 -
> > debian.master/config/armhf/config.flavour.generic | 15 -
> > .../config/armhf/config.flavour.generic-lpae | 15 -
> > debian.master/config/config.common.ubuntu | 13418 -------------------
> > debian.master/config/ppc64el/config.common.ppc64el | 702 -
> > .../config/ppc64el/config.flavour.generic | 3 -
> > debian.master/config/riscv64/config.common.riscv64 | 694 -
> > .../config/riscv64/config.flavour.generic | 3 -
> > debian.master/config/s390x/config.common.s390x | 627 -
> > debian.master/config/s390x/config.flavour.generic | 3 -
> > debian/rules.d/1-maintainer.mk | 13 +-
> > debian/rules.d/2-binary-arch.mk | 6 +-
> > debian/rules.d/4-checks.mk | 5 +-
> > debian/scripts/config-check | 163 -
> > debian/scripts/misc/annotations | 140 +
>
>
>
>
> > def arg_fail(message):
> > print(message)
> > _ARGPARSER.print_usage()
> > exit(1)
> >
> > def do_query(args):
> > a = Annotation(args.file)
> > for config in (args.config, 'CONFIG_' + args.config if args.config else None):
>
>
>
> --query requires a config option __without__ 'CONFIG_' prefix
> but --write requires __with__ 'CONFIG' prefix.
>
> This is really confusing. The policy should be consistent.
>
> Or, you can support both ways.
>
>
> args.config if args.config.startswith('CONFIG') else 'CONFIG_' + args.config.
>
>
> FWIW, the upstream scripts/config supports with/without 'CONFIG_' prefix:
>
> https://github.com/torvalds/linux/blob/v6.0/scripts/config#L60
Done.
Changed in linux-unstable to support both formats (w/wo CONFIG_ prefix),
so we are consistent with upstream.
BTW, I'm also maintaining the annotations script in a separate git repo
(git.launchpad.net/~arighi/+git/annotations-tools), so it's easy to
review/keep track of the latest changes across all kernels.
>
>
>
>
>
>
>
> > def do_import(args):
>
>
> The error messages are the same for the first two cases.
> Unify them, or change the message for each of them.
>
>
> > if args.arch is None:
> > arg_fail('error: --arch and --flavour are required with --import')
>
>
> arg_fail('error: --arch is required with --import')
>
>
> > if args.flavour is None:
> > arg_fail('error: --arch and --flavour are required with --import')
>
>
> arg_fail('error: --flavour is required with --import')
Done (in linux-unstable in the annotations-tools repo).
>
>
>
>
>
>
>
>
> > debian/scripts/misc/kconfig/annotations.py | 242 +
>
>
>
> > class Annotation(Config):
> > """
> > Parse annotations file, individual config options can be accessed via
> > .config[<CONFIG_OPTION>]
> > """
> > def _parse(self, data: str) -> dict:
> > # Parse header
> > self.header = ''
> > for line in data.splitlines():
> > if re.match(r'^#.*', line):
> > m = re.match(r'^# ARCH: (.*)', line)
> > if m:
> > self.arch = list(m.group(1).split(' '))
> > m = re.match(r'^# FLAVOUR: (.*)', line)
> > if m:
> > self.flavour = list(m.group(1).split(' '))
> > self.header += line + "\n"
> > else:
> > break
> >
> > # Skip comments
> > data = re.sub(r'(?m)^\s*#.*\n?', '', data)
>
>
> Since this is done before expanding the 'include' directive,
> comments are only stripped from the top-level annotations file,
> not from included files.
Thanks for catching this! This bug was introduced after a refactoring of
_load and _parse. It should be fixed now in linux-unstable (and the
annotations-tools git repo).
>
>
>
> >
> > # Handle includes (recursively)
>
>
> This does not work recursively.
> Only one-level inclusion is supported.
>
> Let's say, debian.master/config/annotations
> includes "sub", which includes "subsub".
>
> "subsub" will not be included.
Ditto. Fixed now.
>
>
>
>
>
> > self.include = []
> > expand_data = ''
> > for line in data.splitlines():
> > m = re.match(r'^include\s+"?([^"]*)"?', line)
> > if m:
> > self.include.append(m.group(1))
> > include_fname = dirname(abspath(self.fname)) + '/' + m.group(1)
> > include_data = self._load(include_fname)
> > expand_data += include_data + '\n'
> > else:
> > expand_data += line + '\n'
> >
> > # Skip empty, non-policy and non-note lines
> > data = "\n".join([l.rstrip() for l in expand_data.splitlines()
> > if l.strip() and (re.match('.* policy<', l) or re.match('.* note<', l))])
>
>
> You repeat join() and splitlines().
>
> This may not be a big deal on modern computers, but
> it allocates buffers over and over again.
>
>
> Presumably, parsing on-the-fly
> (reading each line, stripping comments and expanding 'include')
> would be more efficient.
Good point. I changed the code moving the parsing when we read each
line, so it should be more efficient now (less memory intensive at
least).
>
>
>
>
>
>
>
> >
> > # Convert multiple spaces to single space to simplifly parsing
> > data = re.sub(r' *', ' ', data)
>
>
> This may potentially break 'string' CONFIG options.
True... but multiple consecutive spaces should be always "fixed" to
single space I think. At least I can't find any reasonable case where
having multiple spaces would be mandatory...
>
>
>
>
>
>
> >
> > # Parse config/note statements
> > config = {}
> > for line in data.splitlines():
> > try:
> > conf = line.split(' ')[0]
> > if conf in config:
> > entry = config[conf]
> > else:
> > entry = {}
> > m = re.match(r'.*policy<(.*)>', line)
> > if m:
> > entry['policy'] = literal_eval(m.group(1))
> > m = re.match(r'.*note<(.*?)>', line)
>
>
> If this detected syntax errors, it would be even better.
>
> Syntax errors such as missing closing angle brackets
> are silently ignored.
We definitely can do better here in terms of syntax checking. For now I
changed the code to detect when a policy or note rule is not properly
closed (triggering a syntax error exception), same when the body of the
rule isn't properly formatted.
Thanks,
-Andrea
More information about the kernel-team
mailing list