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

Unified build script: dry run and unit test #8295

Merged
merged 29 commits into from
Jul 16, 2021

Conversation

andy31415
Copy link
Contributor

Problem

The execution of #8221 is opaque and not tested (except if acutally run).

Would be nice to:
a) show what manual commands could be run to build CHIP (e.g. for build environments where python is not an option or to test things manually
b) unit test that the build script can run and execute things correctly

Change overview

Builds upon #8221 to add:

  • ability to list "what commands would you execute?"
  • add a unit test to validate what a 'build all platform' list of commands would look like

Testing

Unit test created, also manually ran the build.

scripts/build/BUILD.gn Outdated Show resolved Hide resolved
@andy31415 andy31415 marked this pull request as draft July 12, 2021 17:39
@andy31415
Copy link
Contributor Author

Converted to draft until #8221 is merged, since otherwise the diff is too large and inconvenient to review. For reviewers: feel free to review now or wait. Sorry for the large delta.

@todo
Copy link

todo bot commented Jul 13, 2021

this is only for linux. Should be moved to 'HOST' as a platform

# TODO: this is only for linux. Should be moved to 'HOST' as a platform
# to also support building on MacOS
platforms = [Platform.LINUX]
# at this point, at least one of 'platforms' or 'boards' is non-empty
if not boards:
boards = set().union(*[
TargetRelations.BoardsForPlatform(platform) for platform in platforms
])
elif not platforms:
platforms = set().union(


This comment was generated by todo based on a TODO comment in f864715 in #8295. cc @andy31415.

@github-actions
Copy link

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

File Section File VM
chip-shell.elf .flash.text -32 -32
chip-all-clusters-app.elf .flash.text -16 -16
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

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

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

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

sections,vmsize,filesize
.xt.prop._ZN4chip6System5Mutex6UnlockEv,0,108
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,48
[Unmapped],0,16
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,12
.flash.text,-16,-16
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE14_InitChipStackEv,0,-40
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,-80
.xt.lit._ZN4chip6System5Mutex6UnlockEv,0,-128

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

sections,vmsize,filesize

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

sections,vmsize,filesize


@andy31415 andy31415 marked this pull request as ready for review July 14, 2021 14:45
Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

I don't agree with writing this amount of test logic in GN actions.

Tests should be self contained, and feasible to run the tests by themselves, without needing GN to generate a complex set of command lines.

Defining the entire module as inputs to the test also seems wrong, and probably caused by not defining a package. Can we set up a proper package please?

scripts/build/BUILD.gn Outdated Show resolved Hide resolved
scripts/build/BUILD.gn Outdated Show resolved Hide resolved
@andy31415
Copy link
Contributor Author

I don't agree with writing this amount of test logic in GN actions.

Tests should be self contained, and feasible to run the tests by themselves, without needing GN to generate a complex set of command lines.

Defining the entire module as inputs to the test also seems wrong, and probably caused by not defining a package. Can we set up a proper package please?

Updated to use pw_python_package. I feel a bit awkward adding a .txt as a source for that, however I could not find a way for additional deps (especially for tests) and writing the text file into the test itself seemed wrong. Let me know if this needs changes.

scripts/build/BUILD.gn Outdated Show resolved Hide resolved
scripts/build/test.py Outdated Show resolved Hide resolved
scripts/build/BUILD.gn Outdated Show resolved Hide resolved
@andy31415 andy31415 requested a review from mspang July 15, 2021 23:59
@andy31415
Copy link
Contributor Author

@bzbarsky-apple bzbarsky-apple merged commit c51908a into project-chip:master Jul 16, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* Imported a general chip builder script, currently covering a few platforms/boards/apps as a start to have a single build entrypoint

* Move build requirements into global script/requirements.txt so that they get picked up by the bootstrap script

* Update script to assume and require bootstrapping

* Code review comments

* Support building the lock app for ESP32

It turns out that ESP32 lock app is ONLY for devkitc, so
I added application matching logic to include board restrictions.

Tested: lock app compiles and only for devkitc if esp32 platform is
used.

* Remove obsolete todo

* Fix the duplicated accept for efr32 lock app

* Add a dry run option for the build runner, showing what commands would be executed

* Add support for a "test" to validate that the build generator executes commands as expected

* Update the command comparison: output directory of the build script has also to be considered for testing

* Fix some naming and use `get_target_outputs`

* Address some code review comments

* Rename chipbuild to build_examples

* Fixup naming for the unit tests after build script renaming

* Fix names

* Restyle

* Use difflib instead of diff binary for checking changes

* Fix diffs (generator vs lines) and logging

* Start converting python logic from build into pw_python_module

* Tests pass

* Restyle

* Code review comments

* Add comment for all_platform_commands.txt

* Move expected txt data into inputs
@andy31415 andy31415 deleted the 02_dry_run_build_script branch October 28, 2021 14:02
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.

4 participants