Make install_shared_and_static test more robust #2179
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2179 +/- ##
==========================================
+ Coverage 78.95% 79.04% +0.08%
==========================================
Files 611 612 +1
Lines 105551 106065 +514
Branches 14950 14985 +35
==========================================
+ Hits 83341 83835 +494
- Misses 21558 21578 +20
Partials 652 652 ☔ View full report in Codecov by Sentry. |
| @@ -6,7 +6,7 @@ set -exo pipefail | |||
|
|
|||
| source tests/ci/common_posix_setup.sh | |||
|
|
|||
| export CMAKE_BUILD_PARALLEL_LEVEL=${NUM_CPU_THREADS} | |||
| export CMAKE_BUILD_PARALLEL_LEVEL=1 | |||
There was a problem hiding this comment.
Great find, were most of the random CI failures due to race conditions with the CMake building or the invalid file states? Seems like the latter's more of the cause of our issues, but I could be wrong.
There was a problem hiding this comment.
Unsure what the CI failures were because of but my hypothesis is the main culprit is this parallelism. I wasn't able to reproduce the issues.
The other file state stuff and using --fresh is to be more explicit about our expectations of state during the execution.
There was a problem hiding this comment.
Yeah I think --fresh and the file state issues you've discovered seem more like the culprit. CMake parallelism's generally relatively harmless (ninja uses multiple threads by default and make -j(threads) is often used to speed things up).
But nothing to get too fixated over here, this build test is fairly small and fast already.
| ${CMAKE_COMMAND} -H${AWS_LC_DIR} -B${BUILD_DIR} -GNinja -DCMAKE_INSTALL_PREFIX=${INSTALL_DIR} -DBUILD_TESTING=OFF -D${BUILD_SHARED_LIBS} | ||
| sync | ||
|
|
||
| ${CMAKE_COMMAND} --fresh -H${AWS_LC_DIR} -B${BUILD_DIR} -GNinja -DCMAKE_INSTALL_PREFIX=${INSTALL_DIR} -DBUILD_TESTING=OFF -D${BUILD_SHARED_LIBS} |
There was a problem hiding this comment.
Does --fresh behave any differently than removing the entire build folder on line 44?
There was a problem hiding this comment.
It's about having an explicit instruction to cmake to prevent any old cached stuff from being used. Removing the build folder likely takes care of this, but it doesn't hurt to be explicit here. Especially to safeguard against any future changes that might change this expectation
| @@ -6,7 +6,7 @@ set -exo pipefail | |||
|
|
|||
| source tests/ci/common_posix_setup.sh | |||
|
|
|||
| export CMAKE_BUILD_PARALLEL_LEVEL=${NUM_CPU_THREADS} | |||
| export CMAKE_BUILD_PARALLEL_LEVEL=1 | |||
There was a problem hiding this comment.
Yeah I think --fresh and the file state issues you've discovered seem more like the culprit. CMake parallelism's generally relatively harmless (ninja uses multiple threads by default and make -j(threads) is often used to speed things up).
But nothing to get too fixated over here, this build test is fairly small and fast already.
Issues:
P198393553Description of changes:
Making the test more robust to prevent flaky behavior:
Call-outs:
Point out areas that need special attention or support during the review process. Discuss architecture or design changes.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.