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

[.github/workflows] Added pigweed-app related builds to workflows. #4563

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

kkasperczyk-no
Copy link
Contributor

Problem

There is no build validation in CI for pigweed-app, as this app related builds are not ran in the github workflows.

Summary of Changes

  • Modified nrfconnect_example.sh to take board name as an mandatory argument and also allow passing list of additional arguments (e.g. attaching overlays).
  • Added pigweed-app build and lighting-app with rpc.overlay build to the nrfconnect github workflows.
  • Renamed esp_echo_app.sh to the esp_example.sh and modified it to take application name as an argument.
  • Added pigweed-app build to the esp32 github workflow.
  • Renamed Echo App build step to the All Clusters App in the esp32 and qemu github workflows, as in fact that app is being build.

Fixes #4562

Copy link
Contributor

@szatmz szatmz left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

source "$(dirname "$0")/../../scripts/bootstrap.sh"
source "$(dirname "$0")/../../scripts/activate.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't running bootstrap.sh enough?

Copy link
Contributor Author

@kkasperczyk-no kkasperczyk-no Feb 1, 2021

Choose a reason for hiding this comment

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

I think not for pigweed-app, as activate.sh sets proper python version. Without running activate.sh python version is set wrong (I guess it is 2.7 ?) and when compiling pigweed syntax errors appear.

Copy link
Contributor

@szatmz szatmz Feb 1, 2021

Choose a reason for hiding this comment

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

Well, that's weird given that activate.sh is a symlink to bootstrap.sh:

$ ll ./scripts/activate.sh 
lrwxrwxrwx 1 szatmz primarygroup 12 Nov  2 17:48 ./scripts/activate.sh -> bootstrap.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

@kkasperczyk-no can you take a peek at this? Sounds like something isn't working as expected..

Copy link
Contributor

@LuDuda LuDuda left a comment

Choose a reason for hiding this comment

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

Thanks 👍 few comments below.

.github/workflows/examples-nrfconnect.yaml Outdated Show resolved Hide resolved
.vscode/tasks.json Outdated Show resolved Hide resolved
scripts/examples/nrfconnect_example.sh Outdated Show resolved Hide resolved
@woody-apple
Copy link
Contributor

@kkasperczyk-no Note: build fails :(

@kkasperczyk-no
Copy link
Contributor Author

@kkasperczyk-no Note: build fails :(

Yes I know and it also fails for me locally, but I'm not sure if it is related to my change or the pigweed-app for ESP32 build doesn't work and now I added it to CI, so I triggered fail. I contacted with Zoltan, so I have a hope to get some help with that.

@szatmz
Copy link
Contributor

szatmz commented Feb 3, 2021

@szatmz
Copy link
Contributor

szatmz commented Feb 3, 2021

Regarding the ESP32 build breakage:

The problem is that esp_example.sh uses two sdkconfig defaults to seed the sdkconfig for the build: sdkconfig_devkit.defaults and sdkconfig_m5stack.defaults. The directory examples/pigweed-app/esp32 has neither.
The esp_example.sh script should be refactored. It should look for all the sdkconfig*.defaults files and run the build for all of those that actually exist in a particular example app's directory.
Please add this refactoring to your PR. Many thanks in advance!

@szatmz szatmz requested a review from dhrishi February 3, 2021 17:09
@kkasperczyk-no kkasperczyk-no force-pushed the github_workflows_pr branch 2 times, most recently from 84d6f24 to 8432de8 Compare February 5, 2021 09:29
@kkasperczyk-no kkasperczyk-no force-pushed the github_workflows_pr branch 2 times, most recently from 27dfd07 to 106b6c9 Compare February 5, 2021 09:36
There is no build validation in CI for pigweed-app,
as this app related builds are not ran in the github workflows.

* Modified nrfconnect_example.sh to take board name as an mandatory
argument and also allow passing list of additional arguments
(e.g. attaching overlays).
* Added pigweed-app build to the nrfconnect github workflows.
* Renamed esp_echo_app.sh to the esp_example.sh and modified it
to take application name as an argument.
* Added pigweed-app build to the esp32 github workflow.
* Renamed Echo App build step to the All Clusters App in the esp32
and qemu github workflows, as in fact that app is being build.
@kkasperczyk-no
Copy link
Contributor Author

kkasperczyk-no commented Feb 5, 2021

Regarding the ESP32 build breakage:

The problem is that esp_example.sh uses two sdkconfig defaults to seed the sdkconfig for the build: sdkconfig_devkit.defaults and sdkconfig_m5stack.defaults. The directory examples/pigweed-app/esp32 has neither.
The esp_example.sh script should be refactored. It should look for all the sdkconfig*.defaults files and run the build for all of those that actually exist in a particular example app's directory.
Please add this refactoring to your PR. Many thanks in advance!

@szatmz now it's working, but I had to change several things, so please verify if you are ok with them:

  • Added for loop building all sdkconfig*.defaults present in the given app directory
  • Added running activate.sh before building to prevent pigweed-app from failing because of the incorrect python version
  • Changed target name for the esp32 pigweed-app from pw-rpc-app to chip-pigweed-app to be consistent with chip-all-clusters-app.
  • Changed a bit names of output .elf files generated when building sdkconfig.defaults, as it was hard to parametrize it (now it is sdkconfig_devkit-chip-all-clusters-app.elf, sdkconfig_m5stack-chip-all-clusters-app.elf and sdkconfig-chip-all-clusters-app.elf)

@szatmz
Copy link
Contributor

szatmz commented Feb 5, 2021

This looks good to me now, and apparently the build checks are passing too. Thank you Kamil for your work on this. The need to call 'activate.sh' is peculiar, I will ask Pigweed devs about it. Nevertheless that shouldn't hold this PR back from merging.

@andy31415 andy31415 merged commit f4980c1 into project-chip:master Feb 8, 2021
@kkasperczyk-no kkasperczyk-no deleted the github_workflows_pr branch April 16, 2021 08:37
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.

Add building pigweed-app to the CI
7 participants