Relative and deterministic DT comments

[marc-hb]

edtlib.py: make EDT._compat2binding paths relative

This stops leaking private and non-deterministic paths in the (many!)
comments of include/generated/generated_dts_board.conf and
generated_dts_board_unfixed.h

Thanks to [email protected] for explaining how this could be
done.

• Sample lines before:

 # Device tree node: /ioapic@fec00000
 # Binding (compatible = intel,ioapic):                     \
            /home/john/zephyrproject/zephyr/dts/bindings/   \
            interrupt-controller/intel,ioapic.yaml

 /*  Device tree node: /ioapic@fec00000
     Binding (compatible = intel,ioapic):                   \
            /home/john/zephyrproject/zephyr/dts/bindings/   \
            interrupt-controller/intel,ioapic.yaml  */

• Same lines after:

 # Device tree node: /ioapic@fec00000
 # DTS Binding (compatible = intel,ioapic):                 \
            interrupt-controller/intel,ioapic.yaml

 /*  Device tree node: /ioapic@fec00000  */
     DTS Binding (compatible = intel,ioapic):               \
            interrupt-controller/intel,ioapic.yaml  */

Signed-off-by: Marc Herbert [email protected]

3 files changed, 37 insertions(+), 28 deletions(-)
scripts/dts/edtlib.py | 31 ++++++++++++++++++++-----------
scripts/dts/gen_defines.py | 2 +-
scripts/dts/testedtlib.py | 32 ++++++++++++++++----------------

generated_dts_*: replace ${ZEPHYR_BASE}/ with ZEPHYR_BASE in headers

"generated by" comments in the files build/zephyr/include/generated/
generated_dts_board.conf and generated_dts_board_unfixed.h leak absolute
and non-deterministic paths to binding dirs.

When printing those, search for variable ${ZEPHYR_BASE} and replace that
with a literal "ZEPHYR_BASE" constant.

Note: this doesn't deal with dts/bindings out of ZEPHYR_BASE.

Signed-off-by: Marc Herbert [email protected]

1 file changed, 16 insertions(+), 1 deletion(-)
scripts/dts/gen_defines.py | 17 ++++++++++++++++-

[ulfalizer]

Output looks good to me. Just some code nittery. Want to keep main() as short as possible.

[ulfalizer]

Poked around for a while and came up with a helper function that might be handy here:

    out_comment("Generated by gen_defines.py", blank_before=False)
    out_comment("DTS input file: " + args.dts, blank_before=False)
    out_comment("Directories with bindings: " +
                ", ".join(map(relativize, args.bindings_dirs)),
                blank_before=False)

Definition (I put it after parse_args()):

def relativize(path):
    # If 'path' is within $ZEPHYR_BASE, returns it relative to $ZEPHYR_BASE,
    # with a "$ZEPHYR_BASE/..." hint at the start of the string. Otherwise,
    # returns 'path' unchanged.

    if "ZEPHYR_BASE" not in os.environ:
        return path

    try:
        return str("$ZEPHYR_BASE" /
                   Path(path).relative_to(os.environ["ZEPHYR_BASE"]))
    except ValueError:
        # Not within ZEPHYR_BASE
        return path

Has the advantage that gen_defines.py won't crash if $ZEPHYR_BASE is unset, though I don't know if that'd come up.

Thoughts?

[marc-hb]

You can also do:

zbase = os.environ.get("ZEPHYR_BASE")
if not zbase:
   return path

... / Path(path).relative_to(zbase)

[ulfalizer]

Yup, bit neater.

[ulfalizer]

Tinkered a bit and came up with a dict-less version.

        # Used by 'include:'
        self._binding_paths = []
        # Stores paths relative to the binding directory. Used for output, to
        # make it deterministic.
        relpaths = []

        for bdir in bindings_dirs:
            for abspath in _binding_paths(bdir):
                self._binding_paths.append(abspath)
                relpaths.append(os.path.relpath(abspath, bdir))

        self._compat2binding = {}
        for binding_abspath, binding_relpath in zip(self._binding_paths, relpaths):
            with open(binding_abspath, encoding="utf-8") as f:
                contents = f.read()

                # ... (with binding_abspath)

                self._compat2binding[binding_compat, _binding_bus(binding)] = \
                    (binding, binding_relpath)

zip() is for iterating both lists at the same time.

binding_paths() would be similar to the old version:

def _binding_paths(bindings_dir):
    # Returns a list of paths to all .yaml files in one 'bindings_dir'

    binding_paths = []

    for subdir, _, filenames in os.walk(bindings_dir):
        for filename in filenames:
            if filename.endswith(".yaml"):
                binding_paths.append(os.path.join(subdir, filename))

    return binding_paths

Thoughts?

It seems to indirectly remove the ./ at the start of paths too, so would need a test suite update.

[marc-hb]

I'm not sure why this is better than a dict but why not, LGTM.

One of things I liked about the dict is that... it could be done in a small number of changed lines which is easier to rebase constantly. Not the best of reasons, granted :-)

[ulfalizer]

Remembered that DTS is really the name of the .dts source format, so maybe just "Binding" is fine.

[marc-hb]

You basically rewrote the entire patch in the review comments, would you like to submit a new PR and switch author and reviewer roles? I'd just love to get this fixed and don't really care by whom, thanks! :-)

[marc-hb]

You can also do:

zbase = os.environ.get("ZEPHYR_BASE")
if not zbase:
   return path

... / Path(path).relative_to(zbase)

[marc-hb]

I'm not sure why this is better than a dict but why not, LGTM.

One of things I liked about the dict is that... it could be done in a small number of changed lines which is easier to rebase constantly. Not the best of reasons, granted :-)

[ulfalizer]

You've basically rewrote the entire patch in the review comments, would you like to submit a new PR and switch author and reviewer roles? I'd just love to get this fixed and don't really care by whom, thanks! :-)

Will do once I've fixed up some other stuff. Got a bit weird after spending some hours on it, yeah. :P

[marc-hb]

Taken over by @ulfalizer in PR #19444 , thanks!