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.

Sign up for GitHub

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

cmake: compute and display the reproducible checksum by default #51954

Closed
wants to merge 2 commits into from
+2 −0
Closed

cmake: compute and display the reproducible checksum by default #51954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17b8665
cmake: when stripping zephyr.elf, checksum the reproducible output
marc-hb Nov 2, 2022
52a4154
cmake: compute and display the reproducible checksum by default
marc-hb Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1679,6 +1679,7 @@ if(CONFIG_BUILD_OUTPUT_STRIPPED)
$<TARGET_PROPERTY:bintools,strip_flag_infile>${KERNEL_ELF_NAME}
$<TARGET_PROPERTY:bintools,strip_flag_outfile>${KERNEL_STRIP_NAME}
$<TARGET_PROPERTY:bintools,strip_flag_final>
COMMAND ${CMAKE_COMMAND} -E sha256sum ${KERNEL_STRIP_NAME}
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm sure this is a very useful feature, just like printing of memory usage.
But still, printing of memory usage is user configurable, so why should sha256 checksum be always on ?
(I do note it's tied to strip)

zephyr/Kconfig.zephyr

Lines 462 to 463 in 261025a

config OUTPUT_PRINT_MEMORY_USAGE
bool "Print memory usage to stdout"

Should this be a general option that users can enable, so that any output file, bin, hex, s19, etc can have this ?
As a matter of fact, bin files are created per default, and a checksum on those would be more safe than using the elf file for checksumming.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. "Strip" should not be synonymous with "compute & print checksum" and vice versa.

)
list(APPEND
post_build_byproducts
Expand Down
1 change: 1 addition & 0 deletions Kconfig.zephyr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,7 @@ endif # BUILD_OUTPUT_UF2

Copy link
Collaborator

Choose a reason for hiding this comment

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

@marc-hb This change is very useful! Thanks.

Also for the incremental builds test, wouldn't make more sense to touch a header file like ./include/zephyr/kernel.h for example.

config BUILD_OUTPUT_STRIPPED
bool "Build a stripped binary"
default y
Copy link
Collaborator

Choose a reason for hiding this comment

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

do not close a PR that are rejected and open a new PR with same changes, just hoping to get it approve a second time.

Please keep this change in #51737 and let's continue the discussion there.

Copy link
Collaborator Author

@marc-hb marc-hb Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I explained why in #51737 why I closed it and I linked the two both ways - not hiding anything.

In #51737 I made the mistake of stopping half-way, not checksumming the stripped binary. As a consequence all the (few) reviewers missed the point and focused on the wrong thing that I wrongly put in the PR title: the stripped file. The stripped file is a pure, low-level and mostly invisible implementation detail that does not matter. I don't care about the stripped file, it's only a means to an end. Finding reproducibility bugs like #48195 and all the others listed in #14593 is the only thing that matters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As per other commit, rejecting as this is not something that should ever be enabled by default

help
Build a stripped binary zephyr/zephyr.strip in the build directory.
The name of this file can be customized with CONFIG_KERNEL_BIN_NAME.
Expand Down