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

Add support for musl c #7067

Merged
merged 7 commits into from
Nov 1, 2023
Merged

Add support for musl c #7067

merged 7 commits into from
Nov 1, 2023

Conversation

fealebenpae
Copy link
Member

@fealebenpae fealebenpae commented Oct 18, 2023

What, How & Why?

As it says on the tin.

The changes are mostly about things that don't exist in musl, or need configure-time detection:

  • posix_fallocate
  • pthread_getname_np/pthread_setname_np
  • backtrace

And various other quirks like the hardcoded messages produced by strerror that are different between glibc and musl.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@fealebenpae fealebenpae force-pushed the yg/alpine branch 2 times, most recently from 3030c0f to 4f75509 Compare October 18, 2023 17:37
@coveralls-official
Copy link

coveralls-official bot commented Oct 18, 2023

Pull Request Test Coverage Report for Build yavor.georgiev_380

  • 13 of 13 (100.0%) changed or added relevant lines in 4 files are covered.
  • 221 unchanged lines in 19 files lost coverage.
  • Overall coverage decreased (-0.06%) to 91.583%

Files with Coverage Reduction New Missed Lines %
src/realm/aggregate_ops.hpp 1 97.84%
src/realm/util/file.cpp 1 81.69%
src/realm/cluster.cpp 2 80.25%
src/realm/query_engine.hpp 2 94.09%
src/realm/sync/network/websocket.cpp 2 75.36%
src/realm/sync/noinst/server/server_history.cpp 2 67.69%
src/realm/util/fifo_helper.cpp 2 85.71%
src/realm/uuid.cpp 2 97.06%
src/realm/sync/network/network.cpp 3 89.94%
test/test_util_network.cpp 3 97.2%
Totals Coverage Status
Change from base Build 1796: -0.06%
Covered Lines: 230692
Relevant Lines: 251894

💛 - Coveralls

@fealebenpae fealebenpae marked this pull request as ready for review October 18, 2023 20:35
@jedelbo
Copy link
Contributor

jedelbo commented Oct 19, 2023

@fealebenpae There is a test failing on Evergreen (Sync_BundledRealmFile). Could this be caused by the changes here?

@fealebenpae
Copy link
Member Author

Not from what I can tell - the changes here are mostly about low-level things in Core and shouldn’t be affecting existing targets. I will run tests a couple more times and see if I catch a sporadic failure or not.

@fealebenpae
Copy link
Member Author

I'm pretty sure the sync test failures are unrelated. I haven't been able to consistently trigger them across many CI runs and we have sporadic failures in other branches as well.

@michael-wb michael-wb self-requested a review October 31, 2023 15:05
Copy link
Contributor

@michael-wb michael-wb left a comment

Choose a reason for hiding this comment

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

Looks good - I was wondering if we should add a CMake define for Alpine/Musl c, but I don't think we need it.

@@ -64,7 +64,12 @@ TEST(BasicSystemErrors_Category)

TEST(BasicSystemErrors_Messages)
{
#if defined(__linux__) && !defined(__GLIBC__)
// Linux and not glibc implies Musl, which has its own message
Copy link
Contributor

Choose a reason for hiding this comment

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

Of course it does...

@michael-wb
Copy link
Contributor

FYI - The TSAN failure seems to be around the mktime() command: realm-core//test/test_json.cpp:884
Could this have something to do with the different POSIX version?
https://spruce.mongodb.com/task/realm_core_stable_ubuntu2004_tsan_core_tests_patch_e6271d72308b40399890060f58a88cf568c2ee22_6540f3012fbabe7d696a23bf_23_10_31_12_28_50/logs?execution=0&sortBy=STATUS&sortDir=ASC

@fealebenpae
Copy link
Member Author

add a CMake define for Alpine/Musl c

unfortunately there isn't a way I could find to easily detect the C library we're targeting, especially if cross-compilation is involved, and musl deliberately doesn't expose any macros that allow detection like __GLIBC__ for example.

@fealebenpae
Copy link
Member Author

I'll try downgrading the POSIX version and see if that has an impact on the tsan warning.

Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -64,7 +64,12 @@ TEST(BasicSystemErrors_Category)

TEST(BasicSystemErrors_Messages)
{
#if defined(__linux__) && !defined(__GLIBC__)
Copy link
Contributor

Choose a reason for hiding this comment

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

it could be nice to make a REALM_MUSL macro in features.h for readability

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't do that outside of tests as there isn't a sure-fire way to detect musl based on macros alone - in this particular case we know we only build the tests against glibc and musl on Linux, but if someone were to build Core itself with something like uclibc, for example, it would trigger the above code path if it were in Core itself and not just in the tests we build and run ourselves.

@@ -310,6 +312,58 @@ def doCheckInDocker(Map options = [:]) {
}
}

def doCheckAlpine(Map options = [:]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it took CI 18 minutes to compile with musl c. It's not the bottleneck on CI so doesn't matter too much, but that's around double the time for our other linux builders 😄

@fealebenpae fealebenpae merged commit e003bda into master Nov 1, 2023
2 checks passed
@fealebenpae fealebenpae deleted the yg/alpine branch November 1, 2023 17:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants