-
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
Move to CircleCI for Builds and Tests #631
Conversation
Note the most recent run is here: https://app.circleci.com/pipelines/github/woody-apple/connectedhomeip/159/workflows/4e720f20-aa33-48ca-baa9-84005e6664f5 |
integrations/docker/images/chip-build/variants/Dockerfile.esp32-qemu
Outdated
Show resolved
Hide resolved
Sigh, ironically (?) restyled broke the build. Let me dig in. |
Note: Need to improve bash-fu, as restyled is breaking the build script. |
bootstrap takes 56 seconds, vs build takes 17 |
Deployment check repeats work that's already done in CI, tests, as well as bootstrap + build. |
"all tests" take 1 minute +, when localized ones don't. This appears to be repeating NL tests, which we shouldn't in each of our PRs (amongst other things). |
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.
Minor nit: otherwise, nice work!
@@ -0,0 +1,7 @@ | |||
#!/bin/bash | |||
|
|||
if [[ $1 == *"$2"* ]]; then |
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.
what are the asterisks here doing?
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.
They're looking for substrings, it's a way of checking if a build is in a list of things. Maybe not the nicest :)
@@ -31,6 +31,9 @@ third_party/mbedtls/ | |||
# Example specific rules | |||
examples/**/sdkconfig | |||
|
|||
# Temporary Directories | |||
.tmp/ |
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.
could we use build/ for this?
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 an artifact of the circle-ci merge thing. I'd rather keep that contained personally (so I can easily keep it up to date!)
./config | ||
make | ||
|
||
rm -rf ../OpenSSL_1_1_1f.zip |
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.
you could do this after the unzip above?
mkdir -p build/downloads | ||
cd build/downloads | ||
wget https://github.com/openssl/openssl/archive/OpenSSL_1_1_1f.zip | ||
mkdir openssl |
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.
what's the rationale for the extra level of directories?
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.
Trying to figure out a way to have different version of scripts based on platform, not firm on it yet, open to suggestions
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 meant the extra "openssl" directory...
Note Awaiting cost confirmation + enabling CircleCI on the repo before merging. |
@Restyled That's weird, I just merged in master... |
I see, master isn't compliant for spaces... (which I fixed) |
Approved, merging and testing now. |
Problem
Builds have been pretty slow, and get stuck frequently. I have been spending time auditing the various platforms out there, that will allow us to have both per PR and expedient reporting.
CircleCI looks to be a good version of this for us given these things:
• Performance
• Actively developed
• Support has been helpful, and working with me
• Simple configuration, but pretty flexible
Summary of Changes
Move to CircleCI, remove TravisCI config. Builds are speedy! Down rom 25+ minutes to 6-7 mins for a full matrix so far. I suspect I can get these down to 2-3 minutes once we reduce the cost of bootstrap, and redundant tests in makedist, and test-everything.
fixes #401