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

Allow to install on Apple Silicon with Rosetta enabled #34

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kdambekalns
Copy link

@kdambekalns kdambekalns commented Mar 11, 2025

The Issue

Even with Rosetta being enabled on Apple Silicon, the add-on cannot be installed.

How This PR Solves The Issue

If on Apple Silicon with Rosetta, install add-on. If not having Rosetta or if on another ARM system, refuse to install.

Manual Testing Instructions

Running ddev add-on get https://github.com/kdambekalns/ddev-sqlsrv/tarball/task/support-apple-silicon

  • on Apple Silicon with Rosetta should show To make this package work on arm64 machines, Rosetta is used. and install the add-on
  • on Apple Silicon without Rosetta should show This package does not work on arm64 machines, unless Rosetta is enabled., show a hint on how to install that and not install the add-on
  • on any other ARM CPU should show This package does not work on arm64 machines and not install the add-on
  • on a non-ARM CPU should install the add-on

Related Issue Link(s)

Fixes #33

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

Thanks, I'm not a macOS user, so I tested it on Linux amd64, and I confirm that it doesn't break anything for me.

@stasadev stasadev requested a review from rfay March 11, 2025 12:16
@kdambekalns
Copy link
Author

For me (Apple Silicon with Rosetta enabled) it works:

image

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

While we are here, can you please test if arch => uname -m change will work on macOS too.

Asking this because arch is not available on all platforms:

bash: line 17: arch: command not found

@kdambekalns
Copy link
Author

kdambekalns commented Mar 11, 2025

Since arch is only used when "Apple" is detected, that should not break anything, no? And arch checks the given binary by trying to run it with the given architecture, so it's really about testing if it can run (thus checking for Rosetta on Apple Silicon).

That being said, tinking about this made me realize: If you run this on Apple on Intel, it will still show To make this package work on arm64 machines, Rosetta is used. – which is not wrong, but could be omitted. Then again… 🤷‍♂️

@stasadev
Copy link
Member

Okay, thanks.

@rfay
Copy link
Member

rfay commented Mar 11, 2025

Unless I'm missing something, I think this will need something a bit more ambitious. It's at start time that it will fail without rosetta, so there should be a check there.

And note that running most things under Rosetta is not a guaranteed win at all. It's an amazing technology, but it doesn't work everywhere and when it fails it is very mysterious.

Unless this is tested on Colima and Lima and has a check on start that explains what's going on I'd be pretty hesitant.

@kdambekalns
Copy link
Author

It's at start time that it will fail without rosetta, so there should be a check there.

Well, as it is now you can't even install it… and if it indeed fails at start time (even though the check passed when installing), the cause should be obvious.

In the end as it is now the decision is between "not allowed to work, even if it could" and "allow it to work, even if it could fail". Not sure the former is really better. 😉


That being said, for me the installation of the sqlsrv driver into the PHP container did not even work (ODBC version not found, pecl not available), so instead I used a custom "extension" to provide SQL Server. And since I needed an Azurite container, too, it was no big deal.

@rfay
Copy link
Member

rfay commented Mar 11, 2025

I'm not sure I'm following... it sounds like you need a lot more than this add-on to get sqlsrv going properly?

@kdambekalns
Copy link
Author

I'm not sure I'm following... it sounds like you need a lot more than this add-on to get sqlsrv going properly?

See #35 for details – seems to be a different problem.

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

Successfully merging this pull request may close these issues.

Allow setup on Apple Silicon (M1 and up)
3 participants