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

Make builds of esp_app_desc reproducible #257

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

irriden
Copy link
Contributor

@irriden irriden commented Nov 9, 2023

Zero out time and date fields of esp_app_desc_t when esp_idf_app_compile_time_date is not set

@irriden
Copy link
Contributor Author

irriden commented Nov 9, 2023

This is relevant when CONFIG_APP_REPRODUCIBLE_BUILD=y in sdkconfig

@Vollbrecht
Copy link
Collaborator

Ty for your PR, in the end we found the culprits and got it working ;) PR LGTM!

@irriden irriden force-pushed the repro-fix branch 2 times, most recently from efc41c6 to 429fb72 Compare November 9, 2023 20:51
@irriden
Copy link
Contributor Author

irriden commented Nov 9, 2023

@Vollbrecht sorry for the force pushes here, I think this is now correct.

@irriden
Copy link
Contributor Author

irriden commented Nov 9, 2023

@Vollbrecht I believe 3f0c137 is better. Now my question is why CONFIG_APP_COMPILE_TIME_DATE is set when CONFIG_APP_REPRODUCIBLE_BUILD is set:

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/kconfig.html#config-app-compile-time-date

@Vollbrecht
Copy link
Collaborator

do you mean because it says so in the documentation, or is it the real behavior? Could be that the docu is wrong here. You can check what flags are set and not with cargo rustc -- --print cfg

@irriden
Copy link
Contributor Author

irriden commented Nov 10, 2023

do you mean because it says so in the documentation, or is it the real behavior? Could be that the docu is wrong here. You can check what flags are set and not with cargo rustc -- --print cfg

Thanks for the tip! This is the real behavior.

When I remove the CONFIG_APP_REPRODUCIBLE_BUILD from sdkconfig, CONFIG_APP_COMPILE_TIME_DATE is set.

When I set CONFIG_APP_REPRODUCIBLE_BUILD=y, CONFIG_APP_COMPILE_TIME_DATE is set.

If I want a reproducible build, I have to do:

CONFIG_APP_REPRODUCIBLE_BUILD=y
CONFIG_APP_COMPILE_TIME_DATE=n

@Vollbrecht
Copy link
Collaborator

hmm that sounds interesting, for me logically i would assume that CONFIG_APP_REPRODUCIBLE_BUILD will unset CONFIG_APP_COMPILE_TIME_DATE. Maybe its a bug in esp-idf?

Well for our case i would see it more ergonomic to just use the CONFIG_APP_REPRODUCIBLE_BUILD as the base flag to erase the timing data from the app_desc, if the user choose to use our macro for it. Though i have no strong opinion on it.

@irriden
Copy link
Contributor Author

irriden commented Nov 15, 2023

@Vollbrecht I am happy to leave that decision to the maintainers here - just let me know which solution you would merge.

@Vollbrecht
Copy link
Collaborator

i think i would go with CONFIG_APP_REPRODUCIBLE_BUILD last word and the one who will merge is @ivmarkov

@ivmarkov
Copy link
Collaborator

  • CONFIG_APP_REPRODUCIBLE_BUILD appeared since ESP IDF 5.0. It is not present in ESP IDF 4.X
  • CONFIG_APP_COMPILE_TIME_DATE was always there. In ESP IDF 5, if you set CONFIG_APP_REPRODUCIBLE_BUILD, it will unset - in the ESP IDF code though, not in kconfig CONFIG_APP_COMPILE_TIME_DATE

Based on the above we should do exactly what ESP IDF 5 does, here, i.e., only set the date and time when

#[cfg(all(esp_idf_app_compile_time_date, not(esp_idf_app_reproducible_build)))]

... and otherwise keep them zeroed.

@irriden irriden force-pushed the repro-fix branch 2 times, most recently from 5d46da1 to 5d35b4e Compare November 16, 2023 00:44
@irriden
Copy link
Contributor Author

irriden commented Nov 16, 2023

Done. Thanks for your time.

@irriden irriden closed this Nov 16, 2023
@irriden irriden reopened this Nov 16, 2023
Only set time and date fields of `esp_app_desc_t` when
`esp_idf_app_compile_time_date` is set and
`esp_idf_app_reproducible_build` is not set
@Vollbrecht
Copy link
Collaborator

don't forget to run cargo fmt and cargo clippy maybe it wants you to use De Morgan's laws 😸

@irriden
Copy link
Contributor Author

irriden commented Nov 16, 2023

It seems to me both of them are happy with f4d1296 ?

@ivmarkov ivmarkov merged commit 8ce15ff into esp-rs:master Nov 16, 2023
4 checks passed
@irriden irriden deleted the repro-fix branch November 16, 2023 18:39
@kpcyrd
Copy link

kpcyrd commented Nov 29, 2023

Adding onto this, I found that adding CONFIG_APP_EXCLUDE_PROJECT_VER_VAR=y also helps because otherwise the output of git describe --always --tags --dirty gets embedded into your firmware (which you may not want).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants