[PULL] [L/unstable] merge configs into annotations

[Masahiro Yamada]

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







> 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')








>  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.



>
>         # 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.





>         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.







>
>         # Convert multiple spaces to single space to simplifly parsing
>         data = re.sub(r'  *', ' ', data)


This may potentially break 'string' CONFIG options.






>
>         # 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.