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

macos: basic support for darwin builds #20

Draft
wants to merge 6 commits into
base: mariadb-4.x
Choose a base branch
from

Conversation

sitano
Copy link

@sitano sitano commented Jul 31, 2024

It's pretty simple to build Galera Cluster on Darwin (even arm64). Let it be here if anyone will ever need. I will try to rework it in the future into the mergeable form if there is a desire to adopt it.

State

At the moment, I have tested only Cluster Bootstrap, Node Join, and SST/IST streaming. Works on MBP M3.

Copy link
Member

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

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

Thanks for the draft. Most changes are non-invasive so I think they can be merged with a bit of clean-up.

The tcp_info is a bit messy. The usage inside gcomm::AsioTcpSocket::stats and its callers seems to be informational only. If too hard just exclude it for now,

If the tcp_connection_info rtt, rttval, rto map then just using those make sense and seems to be easy.

@@ -104,6 +104,7 @@ target_compile_options(galera_smm
)

if (GALERA_VERSION_SCRIPT)
if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
Copy link
Member

Choose a reason for hiding this comment

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

So this strictly isn't a linux problem, but a gnu linker?

Know why https://github.com/freebsd/freebsd-ports/blob/main/databases/galera26/Makefile is unaffected?

https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_LINKER_ID.html

IF(CMAKE_CXX_LINKER_ID STREQUAL "GNU") ....

Copy link
Author

Choose a reason for hiding this comment

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

agree, will fix

Copy link
Author

Choose a reason for hiding this comment

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

Know why https://github.com/freebsd/freebsd-ports/blob/main/databases/galera26/Makefile is unaffected?

I am not sure, but probably FreeBSD moved to LLVM linker in 12.0 (12.0 (December 11, 2018)) https://man.freebsd.org/cgi/man.cgi?query=ld&apropos=0&sektion=1&manpath=FreeBSD+12.0-RELEASE&arch=default&format=html since when they do fine and this patch codership@e907ec1 landed only in 2019. But I did not check to be sure.

Copy link
Author

Choose a reason for hiding this comment

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

MacOS ld does not support this flag

CMakeLists.txt Outdated Show resolved Hide resolved
cmake/os.cmake Outdated Show resolved Hide resolved
@@ -27,6 +27,102 @@
#include <memory>
#include <string>

#ifdef __APPLE__

struct tcp_info {
Copy link
Member

Choose a reason for hiding this comment

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

if tcp_info is here https://developer.apple.com/documentation/kernel/tcp_info - what does it mean?

So this is a copied list from somewhere?

Copy link
Author

Choose a reason for hiding this comment

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

Speaking of this one, I thought to just reimplement tcp_info from Linux here partially filling it from Apple https://developer.apple.com/documentation/kernel/tcp_connection_info. As it was only a draft I have left only a struct without any fields being filled up. It is copied from https://github.com/torvalds/linux/blob/master/include/uapi/linux/tcp.h. Do you think it makes sense? Or I could cut all unused fields and fill only them?

Copy link
Member

Choose a reason for hiding this comment

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

The only usage in " gcomm::SocketStats gcomm::AsioTcpSocket::stats" includes a small subset namely the tcpi structure:

          ret.rtt            = tcpi.tcpi_rtt;
          ret.rttvar         = tcpi.tcpi_rttvar;
          ret.rto            = tcpi.tcpi_rto;
  #if defined(__linux__)
          ret.lost           = tcpi.tcpi_lost;
  #else   
          ret.lost           = 0;
  #endif /* __linux__ */
          ret.last_data_recv = tcpi.tcpi_last_data_recv;
          ret.cwnd           = tcpi.tcpi_snd_cwnd;

If you find a compatible structure you could alias it, but if there's a difference fields create a minimal structure of only what's needed an populate that. I haven't found the macos function that does this.

@@ -11,8 +11,6 @@

#include "gu_types.h" // bool

#if __unix__
Copy link
Member

Choose a reason for hiding this comment

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

could do #if defined(__unix__) || defined(__APPLE__) as a minimal change without assuming too much about other cases.

Copy link
Author

Choose a reason for hiding this comment

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

yes, I will fix that, it was a hack

Copy link
Author

Choose a reason for hiding this comment

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

#25

@@ -248,11 +248,11 @@ int gu_barrier_init_SYS (gu_barrier_t_SYS *barrier,
errno = EINVAL;
return -1;
}
if(gu_mutex_init_SYS (&barrier->mutex, 0) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

unclear why you swapped arguments here.

pthread_mutex_init seems to be the same on both, mutex then mutex attributed. If there as need to swap them make the gu_mutex_init_SYS macro have arguments that handle these portable.q

Copy link
Author

Choose a reason for hiding this comment

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

there was a patch 09848b6 by Jan from 2021-11-11 14:22:24 +0200 that swapped the order of the arguments to gu_mutex_init_SYS. so now its gu_mutex_init_SYS(const wsrep_mutex_key_t* key, gu_mutex_t_SYS *mutex) so that for keyless mutexes its using (NULL, &mutex).

the macros, if you mean gu_mutex_init is just a debug wrapper over the name, the only that I know: #define gu_mutex_init gu_mutex_init_SYS.

Copy link
Author

Choose a reason for hiding this comment

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

the patch replaced - #define gu_mutex_init_SYS pthread_mutex_init with its own impl that has different args (key, mutex)

Copy link
Author

Choose a reason for hiding this comment

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

#25

@@ -279,13 +279,13 @@ int gu_barrier_wait_SYS (gu_barrier_t_SYS *barrier)
barrier->count = 0;
gu_cond_broadcast_SYS (&barrier->cond);
gu_mutex_unlock_SYS (&barrier->mutex);
return GU_BARRIER_THREAD_SYS;
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Author

Choose a reason for hiding this comment

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

it was introduced by dabe053 8 years ago, but I suppose it never compiled as at that moment there were no GU_BARRIER_THREAD_SYS.

Copy link
Author

Choose a reason for hiding this comment

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

it is synonymous to PTHREAD_BARRIER_SERIAL_THREAD that is must return something non-usual on success (here its -1) for a thread synced with a barrier as per POSIX https://linux.die.net/man/3/pthread_barrier_wait

The constant PTHREAD_BARRIER_SERIAL_THREAD is defined in <pthread.h> and its value shall be distinct from any other value returned by pthread_barrier_wait().

Upon successful completion, the pthread_barrier_wait() function shall return PTHREAD_BARRIER_SERIAL_THREAD for a single (arbitrary) thread synchronized at the barrier and zero for each of the other threads.

Copy link
Author

Choose a reason for hiding this comment

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

(there is no such a constant in the code GU_BARRIER_THREAD_SYS)

Copy link
Author

Choose a reason for hiding this comment

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

#25

{
return -1;
}
if(gu_cond_init_SYS (&barrier->cond, 0) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

#25

@sitano
Copy link
Author

sitano commented Aug 7, 2024

@grooverdan thank your feedback! I will improve everything to be mergeable as at the 1st stage I didn't know if the work worth an effort. Will address / respond to your comments soon.

@sitano
Copy link
Author

sitano commented Aug 7, 2024

@grooverdan wanted to ask if there is any CI pipeline that checks builds under different environments that I could use for dev of this PR? As I am pretty limited at the moment with my options to verify builds for other OSs. or maybe you could suggest what you are using for testing the code on your dev machines to ensure it works for different platforms that must be supported.

@grooverdan
Copy link
Member

Sorry for the delay, what a week:

pull request 20:

https://buildbot.mariadb.org/#/grid?branch=refs%2Fpull%2F20%2Fmerge

Since glibc >=2.17 (2012-12-25), librt is a stub-only library.
(https://abi-laboratory.pro/?view=changelog&l=glibc&v=2.17)

* The `clock_*' suite of functions (declared in <time.h>) is now available
  directly in the main C library.  Previously it was necessary to link with
  -lrt to use these functions.  This change has the effect that a
  single-threaded program that uses a function such as `clock_gettime' (and
  is not linked with -lrt) will no longer implicitly load the pthreads
  library at runtime and so will not suffer the overheads associated with
  multi-thread support in other code such as the C++ runtime library.

Current version on Arch Linux is 2.40.

It is however is required for Debian 10, 11 and Ubuntu 20.04.

Signed-off-by: Ivan Prisyazhnyy <[email protected]>
Signed-off-by: Ivan Prisyazhnyy <[email protected]>
Signed-off-by: Ivan Prisyazhnyy <[email protected]>
calls constructor for a mutex in a struct value init-ed with gu_calloc.

in path `gcs_core_create() -> gcs_group_init()`, the first one allocates
`gcs_core_t* core` with gu_calloc() whereas `gcs_code_t` has
`gcs_group_t group` with `gu::Mutex memb_mtx_`. After memory allocation
gu::Mutex constructor was not called that lead to an error on Darwin in
a call to pthread mutex lock.

Signed-off-by: Ivan Prisyazhnyy <[email protected]>
@sitano
Copy link
Author

sitano commented Sep 17, 2024

any desire to move it forward?
I have a few PRs waiting for the review and approval after which I could add the rest if you are interested.

@sitano
Copy link
Author

sitano commented Sep 17, 2024

we have at the moment to review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants