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 an OpenSSL directory configuration option #1303

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

sodabrew
Copy link
Collaborator

@sodabrew sodabrew commented Jan 19, 2023

CI: Use on MacOS to pick up the correct OpenSSL version. Updates #1301 and #1299

cc @junaruga

@sodabrew
Copy link
Collaborator Author

Need one more edit here, which is that dir_config('openssl') should only activate if specified on the command line, otherwise assume the defaults are OK.

@sodabrew sodabrew force-pushed the junaruga/ci-macos-openssl branch 3 times, most recently from 5bb28f7 to 579691f Compare January 19, 2023 19:36
@junaruga
Copy link
Contributor

junaruga commented Jan 19, 2023

Thanks for the PR! I see this PR is better than mine. Just I have a small proposal in my quick review. With the modification below, I was able to pass the tests without MacOS specific condition: which brew. I tried below on this PR, and I see an issue: CI result.

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index b76f91a..5c25f19 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -27,7 +27,6 @@ jobs:
           - '2.2'
           - '2.1'
         db: ['']
-        ssl: ['']
         include:
           # Comment out due to ci/setup.sh stucking.
           # - {os: ubuntu-18.04, ruby: 2.4, db: mariadb10.1}
@@ -63,4 +62,6 @@ jobs:
         run: echo 'DB=${{ matrix.db }}' >> $GITHUB_ENV
       - run: sudo echo "127.0.0.1 mysql2gem.example.com" | sudo tee -a /etc/hosts
       - run: bash ci/setup.sh
-      - run: bundle exec rake spec -- $([ -x "$(which brew)" ] && echo --with-openssl-dir=$(brew --prefix ${{ matrix.ssl }}))
+      - run: echo "rake_spec_opts=--with-openssl-dir=$(brew --prefix ${{ matrix.ssl }})" >> $GITHUB_ENV
+        if: matrix.ssl
+      - run: bundle exec rake spec -- $rake_spec_opts

@sodabrew
Copy link
Collaborator Author

I made a change in the second to last commit, if you rebase your test branch you'll see if with_config('openssl') changes to if with_config('openssl-dir')

@junaruga
Copy link
Contributor

OK! After rebasng my test branch, I was able to pass the CI with the modification above. 😄

@sodabrew sodabrew force-pushed the junaruga/ci-macos-openssl branch from 95fd24d to 154d061 Compare January 19, 2023 20:49
Starting a few MacOS majors ago, OpenSSL was no longer included in a way that
applications could link against. Even the system Ruby at /usr/bin/ruby was
modified to use a MacOS internal SSL implementation.

The most common workaround is to use Homebrew to install OpenSSL. Using GitHub
Actions as the project's CI tool, we found that both [email protected] and openssl@3
were installed in the default image, and that openssl@3 was returned by default
but this mismatched the version the MySQL client libraries were compiled against.

While the quick workaround might be to look for [email protected] instead of openssl,
a more general improvement is to provide an option for users to specify where
OpenSSL is installed. Indeed this issue has been the cause of dozen so GH
issues and Stack Overflow postings. Hopefully this PR improves the situation
for a broad swath of users!

Unlike the existing option `--with-opt-dir`, the new option `--with-openssl-dir`
will fail if the argument is not a valid path rather than producing unexpected
results at runtime.

This is the default behavior on MacOS:

    --with-openssl-dir=$(brew --prefix openssl)

If you have both [email protected] and openssl@3 installed, be explicit:

    --with-openssl-dir=$(brew --prefix [email protected])

The option is available on all platforms and may be helpful for non-default
OpenSSL installations on Linux or FreeBSD as well.

Co-Authored-By: Jun Aruga <[email protected]>
@sodabrew sodabrew force-pushed the junaruga/ci-macos-openssl branch from 154d061 to 6989e49 Compare January 19, 2023 21:28
@sodabrew sodabrew merged commit a89fd5b into brianmario:master Jan 19, 2023
@sodabrew sodabrew deleted the junaruga/ci-macos-openssl branch January 19, 2023 21:31
philr pushed a commit to philr/mysql2 that referenced this pull request May 21, 2023
…rio#1303)

Starting a few MacOS majors ago, OpenSSL was no longer included in a way that
applications could link against. Even the system Ruby at /usr/bin/ruby was
modified to use a MacOS internal SSL implementation.

The most common workaround is to use Homebrew to install OpenSSL. Using GitHub
Actions as the project's CI tool, we found that both [email protected] and openssl@3
were installed in the default image, and that openssl@3 was returned by default
but this mismatched the version the MySQL client libraries were compiled against.

While the quick workaround might be to look for [email protected] instead of openssl,
a more general improvement is to provide an option for users to specify where
OpenSSL is installed. Indeed this issue has been the cause of many postings on
GH issues and Stack Overflow over the years. Hopefully this PR improves the
situation for a broad swath of users!

Unlike the existing option `--with-opt-dir`, the new option `--with-openssl-dir`
will fail if the argument is not a valid path rather than producing unexpected
results at runtime.

This is the default behavior on MacOS:

    --with-openssl-dir=$(brew --prefix openssl)

If you have both [email protected] and openssl@3 installed, be explicit:

    --with-openssl-dir=$(brew --prefix [email protected])

The option is available on all platforms and may be helpful for non-default
OpenSSL installations on Linux or FreeBSD as well.

Co-authored-by: Jun Aruga <[email protected]>
(cherry picked from commit a89fd5b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants