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

Support arm64/aarch64 Linux and Apple Silicon #12

Merged
merged 6 commits into from
Jan 11, 2022

Conversation

eager-signal
Copy link

@eager-signal eager-signal commented Dec 3, 2021

This cherry-picks the great improvements to building embedded binaries by @n-oden in #11. Still todo:

  • build new binaries
  • update mac build script to statically link to openssl

closes #11

@eager-signal eager-signal marked this pull request as ready for review December 17, 2021 23:00
String machine = input.readLine();
switch (machine) {
case "arm64":
return Architecture.arm64;
Copy link
Author

@eager-signal eager-signal Dec 18, 2021

Choose a reason for hiding this comment

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

Fun from testing: while diagnosing timeouts, I discovered that macOS was always being detected as x86_64, so a native Apple Silicon was necessary but not sufficient.

Copy link

Choose a reason for hiding this comment

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

Oh interesting -- was your jvm an x86_64 binary perchance?

Copy link
Author

@eager-signal eager-signal Jan 7, 2022

Choose a reason for hiding this comment

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

Good question! It was an aarch64 binary, but there was bug in this method: similar to your note on getUnixArchitecture, it was treating all 64-bit architectures as x86_64.

Copy link

Choose a reason for hiding this comment

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

that'll do it :)

echo $ARCH

function copy_openssl_and_remove_dylibs() {
# To make macOS builds more portable, we want to statically link OpenSSL,
Copy link
Author

Choose a reason for hiding this comment

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

more portable

In other words, to not require the user to have openssl dylib from Homebrew installed.

Copy link

Choose a reason for hiding this comment

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

👍

@eager-signal
Copy link
Author

@n-oden thank you again for the the much-improved build script. Let me know if you have any thoughts, and if you want to be added to the contributors in the README.

@n-oden
Copy link

n-oden commented Dec 21, 2021

I'd be delighted!

@n-oden
Copy link

n-oden commented Dec 21, 2021

Mostly LGTM, just a few nitpicks

@eager-signal eager-signal changed the title Support arm64 Linux and Apple Silicon Support arm64/aarch64 Linux and Apple Silicon Jan 7, 2022
@n-oden
Copy link

n-oden commented Jan 10, 2022

Happy new year! I morally bless all parts of this PR that descended from mine. :)

Base automatically changed from chris/tls-support to master January 11, 2022 02:07
n-oden and others added 5 commits January 10, 2022 18:08
- remove references to 32 bit macos/darwin
- add a version of redis compiled for Apple Silicon (darwin/arm64)
- normalize redis server file names
- add linux/arm64 support
- clean up docker/builder scripts
- update README
Bonus: use try-with-resources
@eager-signal
Copy link
Author

Happy new year! I morally bless all parts of this PR that descended from mine. :)

Likewise! And thank you again for your work and support in getting this landed.

@eager-signal eager-signal merged commit 445a8e6 into master Jan 11, 2022
@eager-signal eager-signal deleted the chris/darwin-amd64 branch January 11, 2022 18:35
@sabyasachi90
Copy link

@eager-signal Thanks for the awesome work. Are the changes released with a new binary already ? I am looking to use it for the Apple Silicone support.

@eager-signal
Copy link
Author

@sabyasachi90 it’s not released quite yet, but we should have 0.8.2 published early next week

@jon-signal
Copy link

We've just published the release to Maven Central; it may take a couple hours for everything to move through the upstream plumbing, but it's on its way.

aromanenko-dev pushed a commit to apache/beam that referenced this pull request Feb 10, 2022
The io.oden/embedded-redis artifact was provided by me as part of
landing BEAM-13159, as the previously used version did not use a recent
enough version of Redis to support the XADD/XRANGE command set and also
had a fatal crash bug on darwin/arm64 ("Apple Silicon").

My fork in turn was based on the signal.org artifact, which had a more
recent redis version but was still affected by the crash bug.  Now that
signalapp/embedded-redis#12 has landed, the
io.oden artifact is unlikely to be upgraded further; beam should use the
dependency that is actively maintained.
@radicand
Copy link

radicand commented Mar 8, 2022

This is a great addition - I've noticed incompatibilities however when the host system runs OpenSSL 1.1.1 (or thereabouts) there is a config directive in openssl.cnf of ssl_conf = ssl_sect that when present will cause the redis server to fail to start. This results in a mysterious failure with no error messages. Commenting out the line in openssl.cnf can resolve it (to patch systems/images where you can), however, I think a better solution would be to just ship a newer version of OpenSSL statically linked against the embedded server. Flagging since these changes appear to have introduced the issue.

@eager-signal
Copy link
Author

@radicand Thanks for the note—we can definitely look into it. Can you give a bit more details about your configuration, such as OS, OpenSSL version, etc.?

@jon-signal
Copy link

I've noticed incompatibilities however when the host system runs OpenSSL 1.1.1

@radicand This should be addressed in #15.

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.

5 participants