-
Notifications
You must be signed in to change notification settings - Fork 225
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: Test with musl #682
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #682 +/- ##
==========================================
+ Coverage 77.91% 81.26% +3.34%
==========================================
Files 197 197
Lines 28011 29070 +1059
Branches 5536 4046 -1490
==========================================
+ Hits 21826 23624 +1798
+ Misses 4329 3832 -497
+ Partials 1856 1614 -242 ☔ View full report in Codecov by Sentry. |
Running into this issue it seems: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=764335. |
b445c6d
to
78dddc5
Compare
contrib/build-static.sh
Outdated
REPO_SQLITE="https://github.com/sqlite/sqlite" | ||
REPO_DQLITE="https://github.com/cole-miller/dqlite" | ||
|
||
TAG_MUSL_COMPAT="main" |
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 you please include a test run against the musl tag used in our k8s distributions: https://github.com/canonical/k8s-dqlite/blob/master/hack/env.sh#L13? We might not always be on the same musl version as the juju team.
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.
Juju will be happy to move to the later one, now we know the root cause. We intended to move that originally but ended up locating the segfault.
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.
Sounds good.
92c73e8
to
5f862d2
Compare
I like how using a Dockerfile for this cleans things up. Looking into the segfaults. |
4d602a1
to
d500916
Compare
13b07c3
to
63f3b15
Compare
e10b989
to
f56b473
Compare
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
This is ready. The new The other changes in this PR are tweaks to the test suite to make it musl-friendly. |
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 Cole! Just a couple of comments and questions.
@@ -0,0 +1,231 @@ | |||
#include "../../../src/raft.h" |
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 wonder if there is a way to define the test only for glibc without having to duplicate the rest of the file. The other problem is keeping the CI and the make recipes in sync when adding new tests/binaries.
I see that musl does not define a macro to detect it, but maybe there is a way:
- Either define a macro that gets passed when we want to enable the tests,
gcc -D...
. - Or identify glibc using
__USE_GNU
and use that to guard the tests.
We just have to #ifdef and skip the test early.
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.
The other problem is keeping the CI and the make recipes in sync when adding new tests/binaries.
I'm on this, running subsets of tests seems to be poorly supported by Automake but we can have an environment variable that causes the addrinfo binary to return without running tests
We just have to #ifdef and skip the test early.
The problem is that any tests we want to run cannot be linked into the same binary as test_addrinfo.c
because that file overrides getaddrinfo
and it's impossible to avoid calling it. The only way to do it would be to effectively ifdef out all of test_addrinfo.c
along with the tests that use it. If we went this route, rather than detecting glibc or musl I think it would be better to detect how libc is being linked since that's what makes the difference; we'd need to do some compiler flag sniffing or add a new option in configure.ac. I could go either way on 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.
I've reverted the commit that introduced the new test binary and switched to controlling this with a configure.ac
option (that also causes -all-static
to be passed) and conditional compilation. We can get away without duplicating or complicating the test source files, which is nice, and the tests show up in the output as skipped.
I also added a make target to print the list of test binaries so we don't have to copy the list anywhere.
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
This reverts commit 87e8e6b.
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
…tup" This reverts commit 8737e06.
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 Cole! Just some comments about extra documentation. The code is looking very good and I like the solution a lot.
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
MicroK8s and Juju consume dqlite as a static library build against musl libc, so let's test this in our CI to avoid unexpected breaks like #679.
Signed-off-by: Cole Miller [email protected]