-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[.github/workflows] Added pigweed-app related builds to workflows. #4563
Conversation
842ad3b
to
6dafcf2
Compare
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.
Awesome, thank you!
source "$(dirname "$0")/../../scripts/bootstrap.sh" | ||
source "$(dirname "$0")/../../scripts/activate.sh" |
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.
isn't running bootstrap.sh enough?
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 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.
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.
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
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.
@kkasperczyk-no can you take a peek at this? Sounds like something isn't working as expected..
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.
Thanks 👍 few comments below.
6dafcf2
to
7a4e9ad
Compare
@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. |
https://github.com/project-chip/connectedhomeip/pull/4563/files#diff-470dd5fd8deeed802cd57c2e1d4b0aa41289828b479df1bdfd1cde5cd75c4a48R41 'all-clusters-app' needs replaced with '"$root"', no? |
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. |
1a7b4d4
to
7681b8f
Compare
84d6f24
to
8432de8
Compare
27dfd07
to
106b6c9
Compare
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.
106b6c9
to
5ad3d8c
Compare
@szatmz now it's working, but I had to change several things, so please verify if you are ok with them:
|
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. |
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
Fixes #4562