-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix clear-config in build-all-clusters-app.py #8260
Fix clear-config in build-all-clusters-app.py #8260
Conversation
@@ -54,17 +55,28 @@ def main(): | |||
e = IDFExecutor() | |||
|
|||
if args.clear_config: | |||
old_default_sdkconfig = None | |||
clear_curr = args.clear_config != 'curr' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this line. I'd expect ==
here, not !=
. If there is something deep here that I am missing about what "curr" means, it should at least be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just because the variable is strangely named - "clear_config" basically just re-runs menuconfig starting from the given default. If the default is "curr" (ie, re-run menuconfig, but starting from the current configuration), then we don't need to delete the current configuration and replace the default.
I tried to keep the name because I thought it would be less confusing, but given that this script has been broken for a while now, it would appear I'm likely either the only one or one of the only ones using this. If I renamed this something like menuconfig_with_default, would that work? it would change how the script gets invoked, but I'm not sure it'll actually affect anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fixing the naming would be helpful, yes! I'm fine with a followup for that.
/rebase |
8dfd441
to
3a91b85
Compare
Size increase report for "esp32-example-build" from c1629cc
Full report output
|
Problem
clear-config argument stopped working when we swapped over to the new way of building.
Change overview
Fixes the script so you can run with --clear-config and it will bring up the menuconfig with the proper default.
Testing