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

Darwin support #351

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Darwin support #351

merged 1 commit into from
Mar 22, 2023

Conversation

2010YOUY01
Copy link
Contributor

@2010YOUY01 2010YOUY01 commented Mar 17, 2023

For issue #325
This is an awesome project! I (and possibly other contributors) would like to do development on MacOS X, so I'm trying to make it build on MacOS X.
This patch is currently separated into small commits for review, and will be squashed later if it's possible to merge.

Commits

Fix linker in cmake.

The semantics of check_c_compiler_flag(-Wl,-z,relro HAS_RELRO) is to check if you pass -Wl -z relro to the compiler and if it will throw an error, the compiler will simply pass it to the linker and didn't actually check if linker will throw an error for -z relro option. e.g. I tested check_c_compiler_flag(-Wl,random,var FOO) and FOO is set to true, so the current implementation might have false positive and make the project's cmake fail on MacOS X. Detail also discussed at #325 (comment)

find OpenSSL and headers on Darwin

Setup include path for OpenSSL otherwise openssl/ssl.h can't be found when compiling.
Defined _DARWIN_C_SOURCE to locate system headers
Fixed pgagroal_set_proc_title under MacOS X, I tested it using ps, and works fine.

fix rlim_t type print issue.

rlim_t type seems might be either 4 bytes or 8 bytes on different architectures, and its 8 byte size in MacOS X caused problem.

 error: format specifies type 'long' but the argument has type 'unsigned long long' [-Werror,-Wformat]
      errx(1, "max_connections is larger than the file descriptor limit (%ld available)", flimit.rlim_cur - 30);
                                                                         ~~~              ^~~~~~~~~~~~~~~~~~~~
                                                                         %llu

Check size at compile time seems hard to do, and since pgagroal is currently deployed ok and might be only used for development under MacOS X, here just simply did type casting to fix the error.

Darwin CI setup and formatting.

CI, AUTHOR file, and several formatting issues caught by uncrustify.sh

Testing

Deployed on local machine Darwin Kernel Version 19.6.0, run several simple queries through pgagroal proxy, and tested the concurrent connection using pgbench, no error reported.

@jesperpedersen
Copy link
Collaborator

Nice !

Lets start by doing a squash and force push - plus remove unrelated changes. Feel free to open multiple pull requests...

@2010YOUY01
Copy link
Contributor Author

Squash and force pushed. I forget to mention all modifications are required for the project to build on MacOS X.

@@ -2629,7 +2629,6 @@ transfer_configuration(struct configuration* config, struct configuration* reloa
// changes the pgagroal-cli probably will not be able to connect in any case!
restart_string("unix_socket_dir", config->unix_socket_dir, reload->unix_socket_dir, false);


Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets remove this change from this pull request

@@ -1275,7 +1275,7 @@ ssl_read_message(SSL* ssl, int timeout, struct message** msg)
err = SSL_get_error(ssl, numbytes);
switch (err)
{
case SSL_ERROR_NONE:
case SSL_ERROR_NONE:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets revert this as well

@@ -1371,7 +1371,7 @@ ssl_write_message(SSL* ssl, struct message* msg)

switch (err)
{
case SSL_ERROR_NONE:
case SSL_ERROR_NONE:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets revert this as well

@jesperpedersen
Copy link
Collaborator

Just some white space changes removed in order to minimize the patch...

Signed-off-by: Yongting You <2010youy01@gmail.com>
@2010YOUY01
Copy link
Contributor Author

Sure, removed. And thanks for reviewing!

@jesperpedersen jesperpedersen merged commit 70573f3 into agroal:master Mar 22, 2023
@jesperpedersen
Copy link
Collaborator

Merged.

Thank you for your contribution ! It was very helpful !

Feel free to port the patch to the "sister" projects - pgmoneta and pgexporter :)

@2010YOUY01
Copy link
Contributor Author

I haven't looked into pgmoneta and pgexporter yet. Let's first create issues linked to this PR, so that it will be easier for other contributors to use this patch as a reference if they want to port it themselves.
However, I have already ported and used pgprtdbg. It's very handy for understanding the underlying pg protocol communication of pgagroal, will send a patch soon!

@jesperpedersen
Copy link
Collaborator

@2010YOUY01 No problem !

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

Successfully merging this pull request may close these issues.

2 participants