Sort a few python dicts for deterministic builds, even with python 3.5 and earlier. #14119

marc-hb · 2019-03-06T19:59:06Z

See commit messages.

To reproduce the issue this addresses, run "cmake -B96b_nitrogen" twice with Python 3.5. Observe the order of the flash partition table changing in: "build{1,2}/zephyr/include/generated/generated_dts_board.conf" and "generated_dts_board_unfixed.h" Dictionaries are iterated in a random order by Python 3.5 and before. This could have caused "Unstable" CI in PR zephyrproject-rtos#13921 and maybe others. Anyway we want builds to be determimistic by default. Explicit randomness can be added for better coverage but not by default. Signed-off-by: Marc Herbert <[email protected]>

ulfalizer · 2019-03-06T20:01:31Z

scripts/dts/extract_dts_includes.py

-    for node_path in reduced.keys():
+    # sorted() otherwise Python < 3.6 randomizes the order of the flash
+    # partition table
+    for node_path in sorted(reduced.keys()):


Looks like I missed this earlier, but could get rid of the .keys(). x in dict, for x in dict, sorted(dict), etc., always uses the keys.

Agreed but I'd like to keep this particular commit as minimal as possible :-) Not just for aesthetic reasons but also cherry-picking for testing / backporting etc.

Alright, fine with me.

ulfalizer · 2019-03-06T20:04:55Z

scripts/gen_kobject_list.py

        eh = ElfHelper(args.kernel, args.verbose, kobjects, subsystems)
        syms = eh.get_symbols()
        max_threads = syms["CONFIG_MAX_THREAD_BYTES"] * 8
        objs = eh.find_kobjects(syms)
+        if len(objs) == 0:


Can do if not objs:.

ulfalizer · 2019-03-06T20:05:04Z

scripts/gen_priv_stacks.py

    eh = ElfHelper(args.kernel, args.verbose, kobjects, [])
    syms = eh.get_symbols()
    max_threads = syms["CONFIG_MAX_THREAD_BYTES"] * 8
    objs = eh.find_kobjects(syms)
+    if len(objs) == 0:


Ditto here.

ulfalizer · 2019-03-06T20:12:21Z

scripts/gen_priv_stacks.py

@@ -101,10 +101,14 @@ def parse_args():
 def main():
    parse_args()

+    assert args.kernel, "--kernel ELF required for --gperf-output"


There's a required=True on the parser.add_argument() that adds --kernel, so this might not be needed.

You're right, I saw the mostly duplicated main() so I blindly copied the assert from the other script. Will remove.

ulfalizer · 2019-03-06T20:15:56Z

scripts/gen_kobject_list.py

@@ -269,10 +276,14 @@ def main():
    parse_args()

    if args.gperf_output:
+        assert args.kernel, "--kernel ELF required for --gperf-output"


Maybe

if not args.kernel: sys.exit("--kernel must be passed if --gperf-output is")

would be a bit more purist.

It's more of a usage error than a runtime error.

It is a usage error but... a pretty stupid, "trial-and-error" one so I didn't want to spend too many lines of code on it.

codecov-io · 2019-03-06T20:37:39Z

Codecov Report

Merging #14119 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #14119   +/-   ##
=======================================
  Coverage   52.68%   52.68%           
=======================================
  Files         307      307           
  Lines       45473    45473           
  Branches    10530    10530           
=======================================
  Hits        23956    23956           
  Misses      16640    16640           
  Partials     4877     4877

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97f4265...1d3dff3. Read the comment docs.

andrewboie

other than the minor issues @ulfalizer noted this LGTM

Dictionaries are iterated in a random order by Python 3.5 and before. This could have caused "Unstable" CI in PR zephyrproject-rtos#13921 and maybe others. Anyway we want builds to be determimistic by default. Explicit randomness can be added for better coverage but not by default. 1. When running "make kobj_types_h_target" repeatedly one can observe that the following .h files keep changing in build/zephyr/include/generated/: - kobj-types-enum.h - otype-to-str.h - otype-to-size.h Switching kobjects to OrderedDict makes these 3 .h files deterministic. 2. When running this test repeatedly with CONFIG_USERSPACE=y: rm build/zephyr/*.gperf && make -C build obj_list ... the dict used for --gperf-output seems to be deterministic, probably because its keys are all integers (memory addresses). However we can't take that for granted with Python < 3.6 so out of caution also switch the output of find_objects() in elf_helper.py to a sorted OrderedDict. PS: I would normally prefer official Python documentation to StackOverflow however this one is a good summary and has all the multiple pointers to the... official Python documentation. Signed-off-by: Marc Herbert <[email protected]>

ulfalizer

Still think the assert for reporting a usage error looks confusing, but won't block over it.

marc-hb · 2019-03-07T04:30:54Z

Thanks for the very prompt reviews. For the record this is a "screenshot" of a typical diff: https://gist.github.com/nashif/675dd70be922f0200c181d6a65baeafb

There's also one last crypto.mbedtls/zephyr/include/generated/app_smem.ld randomness there which no one has reproduced, maybe addressed elsewhere?

marc-hb requested review from agross-oss and andrewboie as code owners March 6, 2019 19:59

ulfalizer reviewed Mar 6, 2019

View reviewed changes

ulfalizer suggested changes Mar 6, 2019

View reviewed changes

marc-hb requested review from galak, lpereira and chunlinhan March 6, 2019 20:27

andrewboie approved these changes Mar 6, 2019

View reviewed changes

nashif approved these changes Mar 6, 2019

View reviewed changes

marc-hb force-pushed the deterministic-builds branch from 0bf4e64 to 1d3dff3 Compare March 6, 2019 23:15

marc-hb requested a review from ulfalizer March 7, 2019 00:59

ulfalizer approved these changes Mar 7, 2019

View reviewed changes

nashif merged commit f78288b into zephyrproject-rtos:master Mar 7, 2019

marc-hb mentioned this pull request Mar 16, 2019

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

Closed

marc-hb deleted the deterministic-builds branch January 12, 2021 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort a few python dicts for deterministic builds, even with python 3.5 and earlier. #14119

Sort a few python dicts for deterministic builds, even with python 3.5 and earlier. #14119

marc-hb commented Mar 6, 2019

ulfalizer Mar 6, 2019

marc-hb Mar 6, 2019 •

edited

Loading

ulfalizer Mar 6, 2019

ulfalizer Mar 6, 2019

marc-hb Mar 6, 2019

ulfalizer Mar 6, 2019

marc-hb Mar 6, 2019

ulfalizer Mar 6, 2019 •

edited

Loading

marc-hb Mar 6, 2019

ulfalizer Mar 6, 2019

marc-hb Mar 6, 2019

codecov-io commented Mar 6, 2019 •

edited

Loading

andrewboie left a comment

ulfalizer left a comment

marc-hb commented Mar 7, 2019

Sort a few python dicts for deterministic builds, even with python 3.5 and earlier. #14119

Sort a few python dicts for deterministic builds, even with python 3.5 and earlier. #14119

Conversation

marc-hb commented Mar 6, 2019

Choose a reason for hiding this comment

marc-hb Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulfalizer Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 6, 2019 • edited Loading

Codecov Report

andrewboie left a comment

Choose a reason for hiding this comment

ulfalizer left a comment

Choose a reason for hiding this comment

marc-hb commented Mar 7, 2019

marc-hb Mar 6, 2019 •

edited

Loading

ulfalizer Mar 6, 2019 •

edited

Loading

codecov-io commented Mar 6, 2019 •

edited

Loading