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

Kconfig: enable CONFIG_BUILD_OUTPUT_STRIPPED by default #51737

Closed
wants to merge 1 commit into from
Closed

Kconfig: enable CONFIG_BUILD_OUTPUT_STRIPPED by default #51737

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 Kconfig.zephyr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ endif # BUILD_OUTPUT_UF2

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.

This helps investigation of reproducibility issues like the one recently fixed by commit

So why should this be enabled by default for every build when it provides no use to nearly every zephyr user and those that want to use it can enable it manually with menuconfig or in their prj.conf?

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree with @nordicjm .

If someone want to enable this per default, then they can easily do this in a samples prj.conf, in their custom board Kconfig, in their custom Zephyr module, etc.

I'm not convinced this should default to y for everyone.

Copy link
Collaborator Author

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

Choose a reason for hiding this comment

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

when it provides no use to nearly every zephyr user and those that want to use

This PR has two "insurance" purposes that you have not yet commented on. It's normal to think you don't need insurance against a risk you have never considered.

  1. Reproducible builds and security, see a sample checksumming use in xtensa-build-zephyr.py: install and checksum .elf, .lst and other files thesofproject/sof#6501. Security is not something users generally perceive as "useful", usually the opposite. This PR is special because it will help with security without anyone even noticing - until they are actually targeted by some supply-chain attack and then realize the value.

  2. Making sure everyone in some "task force" is using the exact same binary when trying to reproduce some very elusive failure. Again this is the kind of feature that you don't know you need until you're in this sort of situation. By that time people have much more urgent things to do than figure out how to tweak everyone's configuration and the configuration of automated builds - with the risk of introducing other changes. With a reproducible build catching issues like f896fc2 can become routine instead and happen long before any "task force" is started.

those that want to use it can enable it manually with menuconfig or in their prj.conf?

That's too late when trying to reproduce binaries already released and/or built by CI. It's also inconvenient when trying to compare binaries across multiple people including validation people or other non-developers who have never configured anything in Zephyr.

Finally let's also have a look at a sample, current, top-level build/zephyr/ directory, see below (I omitted the subdirectories). I can see 3 .elf files and 4 .map files. besides to the build system itself, which of these files are useful and to whom? What about all the other files?

This PR would add this reproducible file for a totally negligible build time and disk space

 43572 Nov  1 22:57 zephyr.strip


$ west build -b qemu_x86 samples/hello_world/

ls -lS build/zephyr/

440334 Nov  1 22:52 zephyr.lst
414364 Nov  1 22:52 zephyr.elf
414160 Nov  1 22:52 zephyr_pre1.elf
413544 Nov  1 22:52 zephyr_pre0.elf
371436 Nov  1 22:52 libzephyr.a
231181 Nov  1 22:52 zephyr_final.map
231181 Nov  1 22:52 zephyr.map
230997 Nov  1 22:52 zephyr_pre1.map
230116 Nov  1 22:52 zephyr_pre0.map
224567 Nov  1 22:52 edt.pickle
 21340 Nov  1 22:52 dts.cmake
 11288 Nov  1 22:52 linker.cmd
 11228 Nov  1 22:52 linker_zephyr_pre0.cmd
 11228 Nov  1 22:52 linker_zephyr_pre1.cmd
  4618 Nov  1 22:52 linker_zephyr_pre0.cmd.dep
  4618 Nov  1 22:52 linker_zephyr_pre1.cmd.dep
  4606 Nov  1 22:52 linker.cmd.dep
  4423 Nov  1 22:52 zephyr.dts.pre
  3558 Nov  1 22:52 zephyr.stat
  3368 Nov  1 22:52 zephyr.dts
   885 Nov  1 22:52 dev_handles.c
   345 Nov  1 22:52 zephyr.dts.d

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your comments do not convince me of anything useful about this being the default. In the 5/6 years I've used zephyr, I've never needed this feature, as for right now I absolutely do not want to use this feature, you are trying to force a default on everyone and you're not able to convince me with the above that any random zephyr user would get any benefit from this at all.

Copy link
Collaborator Author

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

Choose a reason for hiding this comment

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

I understand this has no value for you and I'm sorry you don't see the generic problems this helps with. However this is mysterious to me:

I absolutely do not want to use this feature,

It's mysterious because most people would never notice this change. Adding a (reproducible at last) kilobytes-big .elf file in the megabytes-big build/ directory without affecting any other existing file is really not a "feature".

So I understand that you don't see any value but I don't understand why you seem to care so much. What problem(s) does this cause in practice for you?

Copy link
Member

@nashif nashif Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I do not see any value of enabling this by default as well, this does not give us anything, if you need the stripped binary, you can just enable it, if you want to verify reproducibility and check for reproducibility, you can enable it, but build a strpped version of each app/test by default is not going to guarantee reproducibility, so it is some kind of false advertisement: "zephyr.strip is deterministic / reproducible" (If the binary is not reproducible in the first place, the stripped output will not be....)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's mysterious because most people would never notice this change. Adding a (reproducible at last) kilobytes-big .elf file in the megabytes-big build/ directory without affecting any other existing file is really not a "feature".

Oh the change would be noticeable, especially for CI given that thousands of tests run, adding a completely pointless additional build stage and output file would start having a time impact on all CIs runs for all PRs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh the change would be noticeable, especially for CI given that thousands of tests run, adding a completely pointless additional build stage and output file would start having a time impact on all CIs runs for all PRs.

This is not what I've measured, what makes you think that?

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