scripts: dts: Output paths relative to $ZEPHYR_BASE #19444

ulfalizer · 2019-09-27T21:21:32Z

Requested by Marc Herbert. Makes the output deterministic as long as all
binding directories are within $ZEPHYR_BASE (and a bit less spammy too).

Example output for header:

Before:

/*  Directories with bindings: /home/ulf/z/z/dts/bindings  */
...
/*  Binding (...): /home/ulf/.../arm,v7m-nvic.yaml  */

After:

/*  Directories with bindings: $ZEPHYR_BASE/dts/bindings  */
...
/*  Binding (...): $ZEPHYR_BASE/dts/.../arm,v7m-nvic.yaml  */

marc-hb

Tested and works! See inline.

marc-hb · 2019-09-27T22:41:39Z

scripts/dts/edtlib.py

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


This part of the code is confusing because there are not just two but three parts involved in the paths here:

ZEPHYR_BASE

dts/bindings

some/dir/file.yaml

I think I remember what this particular relpath() does but barely and that's after me having designed, written and thoroughly tested the first version of this in PR #19333! So imagine someone new to this code...

=> this deserves: - either more comments clarifying what is relative to what (some dts/bindings example wouldn't hurt); - or super-smart variable names, much more elaborated than these. Adding comments seems obviously faster and easier ("If I Had More Time, I Would Have Written a Shorter Letter - Blaise Pascal)

Lengthened the variable names and added a comment. The relpath() is to remove the path to the binding directory, turning e.g. path/to/zephyr/dts/bindings/sub/foo.yaml into sub/foo.yaml.

marc-hb · 2019-09-27T22:52:12Z

scripts/dts/gen_defines.py

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


While the other fix is really under-commented, maybe this one is a bit over-commented?

Also why no pydoc syntax? (if anything)

I usually do doc comments unless it's part of the API for some library. Another way of marking it internal, kinda. If it was an internal part of a library (gen_defines.py is just a script), I'd put _ before the name too.

Not saying that's the one true way to do it, but there's a method to the madness. ;)

pabigot

I think replacing a path prefix that matches ${ZEPHYR_BASE} with ${ZEPHYR_BASE} (with or without braces) is a good idea, for privacy as well as reproducibility.

But ${DTS_ROOT_BINDINGS} may contain several directories, and the same compatible may appear in multiple locations (e.g. when being overridden for an application or a board). The current tooling appears to mishandle this (#19536).

This patch also removes everything but the relative path, losing the information about exactly which file provided the binding for the compatible. That's not really acceptable, IMO.

So, +1 for the ${ZEPHYR_BASE} piece, -1 for the relpath piece. We should get:

/*  Directories with bindings: $ZEPHYR_BASE/dts/bindings  */
...
/*  Binding (...): $ZEPHYR_BASE/dts/bindings/interrupt-controller/arm,v7m-nvic.yaml  */

ulfalizer · 2019-10-02T02:14:35Z

So, +1 for the ${ZEPHYR_BASE} piece, -1 for the relpath piece. We should get:
/*  Directories with bindings: $ZEPHYR_BASE/dts/bindings  */
...
/*  Binding (...): $ZEPHYR_BASE/dts/bindings/interrupt-controller/arm,v7m-nvic.yaml  */

Wonder how to deal with binding directories outside $ZEPHYR_BASE in that case though.

One thing I considered was making it foo/bar/binding.yaml when you pass --binding-dir some/path/foo (including the directory name). Don't know if that's acceptable to @marc-hb though.

Not guaranteed to be unique either I guess.

ulfalizer · 2019-10-02T03:10:58Z

Hmm... seems the Directories with bindings: stuff would include those paths directly anyway though, so might not be any worse to just add it.

Think I'll move it all to gen_defines.py.

marc-hb · 2019-10-02T03:35:59Z

Don't know if that's acceptable to @marc-hb though.

From a build reproducibility perspective (= the initial rationale for this), anything that generates the exact same files when building from two different git clones is acceptable.

For instance, no comment at all is acceptable ;-)

pabigot · 2019-10-02T09:59:43Z

For instance, no comment at all is acceptable ;-)

I would prefer that to providing misleading/incomplete information, even though it makes the system less usable. I guess it's a question of whether build reproducibility is a more important criterion than "providing information that's helpful to the developer".

Another approach would be to provide a hook for a post-processing step that strips out irrelevant differences when performing a build that must be "reproducible".

ulfalizer · 2019-10-02T19:42:23Z

Updated it to do the thing @pabigot suggested (just stripping $ZEPHYR_BASE from all binding paths within the Zephyr repo, with changes to just gen_defines.py).

That makes it deterministic as long as there are no binding directories outside $ZEPHYR_BASE. I realized the old version was also non-deterministic when there are binding paths outside $ZEPHYR_BASE though, since it shows the paths in Directories with bindings: ... in that case.

@marc-hb
Are you fine with this? Something fancier could be done later to make it deterministic for binding directories outside $ZEPHYR_BASE too, if needed.

Requested by Marc Herbert. Makes the output deterministic as long as all binding directories are within $ZEPHYR_BASE (and a bit less spammy too). Example output for header: Before: /* Directories with bindings: /home/ulf/z/z/dts/bindings */ ... /* Binding (...): /home/ulf/.../arm,v7m-nvic.yaml */ After: /* Directories with bindings: $ZEPHYR_BASE/dts/bindings */ ... /* Binding (...): $ZEPHYR_BASE/dts/.../arm,v7m-nvic.yaml */ Signed-off-by: Ulf Magnusson <[email protected]>

marc-hb · 2019-10-02T20:08:12Z

Are you fine with this?

I will test and get back to you.

Something fancier could be done later to make it deterministic for binding directories outside $ZEPHYR_BASE too, if needed.

Some sort of WEST_TOP (#19440) would be a great next step. Beyond that would be even better but I'm not looking that far TBH. Fixing everything inside ZEPHYR_BASE will already make a huge difference.

marc-hb

works4me

pabigot

This seems to do what's expected on the multi-directory test case I have.

ulfalizer requested a review from galak as a code owner September 27, 2019 21:21

ulfalizer requested a review from marc-hb September 27, 2019 21:21

marc-hb reviewed Sep 27, 2019

View reviewed changes

marc-hb mentioned this pull request Sep 27, 2019

Relative and deterministic DT comments #19333

Closed

ulfalizer force-pushed the deterministic-paths branch from 46cb409 to 78884ff Compare September 27, 2019 23:52

marc-hb approved these changes Sep 28, 2019

View reviewed changes

marc-hb requested review from pabigot, mbolivar and carlescufi October 2, 2019 00:02

pabigot requested changes Oct 2, 2019

View reviewed changes

marc-hb mentioned this pull request Oct 2, 2019

meta issue for reproducible builds (was: tests/determinism.sh) #14593

Closed

marc-hb requested a review from erwango October 2, 2019 14:09

ulfalizer force-pushed the deterministic-paths branch from 78884ff to 7901ffd Compare October 2, 2019 19:36

ulfalizer changed the title ~~scripts: dts: Output paths relative to $ZEPHYR_BASE and binding dirs.~~ scripts: dts: Output paths relative to $ZEPHYR_BASE Oct 2, 2019

ulfalizer force-pushed the deterministic-paths branch from 7901ffd to 2ecd481 Compare October 2, 2019 19:44

marc-hb approved these changes Oct 2, 2019

View reviewed changes

ulfalizer requested a review from pabigot October 3, 2019 04:18

pabigot approved these changes Oct 3, 2019

View reviewed changes

galak merged commit 178f16f into zephyrproject-rtos:master Oct 3, 2019

galak deleted the deterministic-paths branch October 3, 2019 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: dts: Output paths relative to $ZEPHYR_BASE #19444

scripts: dts: Output paths relative to $ZEPHYR_BASE #19444

ulfalizer commented Sep 27, 2019 •

edited

Loading

marc-hb left a comment

marc-hb Sep 27, 2019

ulfalizer Sep 27, 2019

marc-hb Sep 27, 2019

ulfalizer Sep 27, 2019 •

edited

Loading

pabigot left a comment

ulfalizer commented Oct 2, 2019 •

edited

Loading

ulfalizer commented Oct 2, 2019

marc-hb commented Oct 2, 2019

pabigot commented Oct 2, 2019 •

edited

Loading

ulfalizer commented Oct 2, 2019

marc-hb commented Oct 2, 2019

marc-hb left a comment

pabigot left a comment

scripts: dts: Output paths relative to $ZEPHYR_BASE #19444

scripts: dts: Output paths relative to $ZEPHYR_BASE #19444

Conversation

ulfalizer commented Sep 27, 2019 • edited Loading

marc-hb left a comment

Choose a reason for hiding this comment

marc-hb Sep 27, 2019

Choose a reason for hiding this comment

ulfalizer Sep 27, 2019

Choose a reason for hiding this comment

marc-hb Sep 27, 2019

Choose a reason for hiding this comment

ulfalizer Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

pabigot left a comment

Choose a reason for hiding this comment

ulfalizer commented Oct 2, 2019 • edited Loading

ulfalizer commented Oct 2, 2019

marc-hb commented Oct 2, 2019

pabigot commented Oct 2, 2019 • edited Loading

ulfalizer commented Oct 2, 2019

marc-hb commented Oct 2, 2019

marc-hb left a comment

Choose a reason for hiding this comment

pabigot left a comment

Choose a reason for hiding this comment

ulfalizer commented Sep 27, 2019 •

edited

Loading

ulfalizer Sep 27, 2019 •

edited

Loading

ulfalizer commented Oct 2, 2019 •

edited

Loading

pabigot commented Oct 2, 2019 •

edited

Loading