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

Fix clear-config in build-all-clusters-app.py #8260

Merged

Conversation

cecille
Copy link
Contributor

@cecille cecille commented Jul 9, 2021

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

  • ran script with --clear-config and various arguments.

@@ -54,17 +55,28 @@ def main():
e = IDFExecutor()

if args.clear_config:
old_default_sdkconfig = None
clear_curr = args.clear_config != 'curr'
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@bzbarsky-apple
Copy link
Contributor

/rebase

@woody-apple woody-apple force-pushed the fix_all_clusters_script_clear branch from 8dfd441 to 3a91b85 Compare July 12, 2021 22:26
@github-actions
Copy link

Size increase report for "esp32-example-build" from c1629cc

File Section File VM
chip-lock-app.elf .flash.text -64 -64
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
[Unmapped],0,64
.flash.text,-64,-64

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize


@andy31415 andy31415 merged commit 5cb8c00 into project-chip:master Jul 13, 2021
@cecille cecille deleted the fix_all_clusters_script_clear branch July 23, 2021 19:34
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants