Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

file2hex.py: new --gzip-mtime option that defaults to zero + test #15043

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Mar 30, 2019

file2hex.py: new --gzip-mtime option that defaults to zero + test

This makes the output of file2hex.py deterministic by default while also
letting the user set the mtime in the gzip header manually if desired.

Use the option without any argument to restore the previous behavior
that sets the current (and obviously changing) "now" timestamp.

To test: ./sanitycheck --tag gen_inc_file

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

@marc-hb marc-hb requested a review from jukkar March 30, 2019 07:29
@marc-hb marc-hb added area: Test Framework Issues related not to a particular test, but to the framework instead area: Build System area: Testing area: Tests Issues related to a particular existing or missing test Enhancement Changes/Updates/Additions to existing features labels Mar 30, 2019
@marc-hb marc-hb requested review from SebastianBoe and nashif March 30, 2019 07:32
@codecov-io
Copy link

Codecov Report

Merging #15043 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15043      +/-   ##
==========================================
+ Coverage   52.93%   52.94%   +0.01%     
==========================================
  Files         309      309              
  Lines       45268    45273       +5     
  Branches    10451    10456       +5     
==========================================
+ Hits        23961    23969       +8     
+ Misses      16542    16536       -6     
- Partials     4765     4768       +3
Impacted Files Coverage Δ
subsys/testsuite/ztest/src/ztest.c 68.59% <0%> (-4.14%) ⬇️
drivers/clock_control/nrf_power_clock.c 50% <0%> (-2.11%) ⬇️
boards/posix/nrf52_bsim/argparse.c 46.98% <0%> (+15.66%) ⬆️

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 c6249ac...d3fbbaa. Read the comment docs.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -44,6 +46,7 @@ def main():
with io.BytesIO() as content:
with open(args.file, 'rb') as fg:
with gzip.GzipFile(fileobj=content, mode='w',
mtime=args.gzip_mtime,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't imagine that the embedded FW would be interested in the timestamp of the host system source file.

IMHO we should simplify by always setting mtime=0. Then the next user that wants reproducible FW won't have to discover that he needs to set --gzip-mtime=0.

Copy link
Collaborator Author

@marc-hb marc-hb Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do unless anyone objects. In that case I will also simplify the gen_inc test that currently ignores the specific bytes where the timestamp is located.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK mtime=0 will still allocate space for a zero'd timestamp, so the test should/can continue to ignore the timestamp. Or it could assert it to be zero if it wanted to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the latter so the test tests the new option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and ready for re-review.

This makes the output of file2hex.py deterministic by default while also
letting the user set the mtime in the gzip header manually if desired.

Use the option without any argument to restore the previous behavior
that sets the current (and obviously changing) "now" timestamp.

To test:  ./sanitycheck --tag gen_inc_file

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb force-pushed the gen_inc_file-gzip-mtime branch from d3fbbaa to ff1006d Compare April 5, 2019 00:07
@marc-hb marc-hb changed the title file2hex.py: new --gzip-mtime option; makes --tag gen_inc_file test deterministic file2hex.py: new --gzip-mtime option that defaults to zero + test Apr 5, 2019
@marc-hb marc-hb requested review from jukkar and dbkinder April 5, 2019 00:43
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@nashif nashif merged commit 3061c92 into zephyrproject-rtos:master Apr 17, 2019
@marc-hb marc-hb deleted the gen_inc_file-gzip-mtime branch April 20, 2019 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Test Framework Issues related not to a particular test, but to the framework instead area: Tests Issues related to a particular existing or missing test Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants