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 --arch global option to support arm64 macOS via Rosetta2 #245

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

errikos
Copy link
Contributor

@errikos errikos commented Oct 1, 2021

Right now, mx refuses to run on M1 Macs (#232).

Allow to specify a custom architecture target. Useful for arm64-based Apple computers, until all used native libraries are released for arm64.

Example usage:
mx --arch amd64 ...

@graalvmbot
Copy link
Collaborator

Hello Ergys Dona, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address ergys -(dot)- dona -(at)- gmail -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@errikos
Copy link
Contributor Author

errikos commented Oct 1, 2021

Hello Ergys Dona, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address ergys -(dot)- dona -(at)- gmail -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

One of my organisations here at GitHub (HexHive at EPFL - my employer, https://github.com/HexHive) has signed the CLA.

@graalvmbot
Copy link
Collaborator

Hello Ergys Dona, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address ergys -(dot)- dona -(at)- epfl -(dot)- ch. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@errikos
Copy link
Contributor Author

errikos commented Oct 2, 2021

As already stated above, one of my organisations here at GitHub (HexHive at EPFL - my employer, https://github.com/HexHive) has signed the CLA, so I have been told that it should not be necessary that I sign it individually.

@dougxc
Copy link
Member

dougxc commented Oct 4, 2021

This change is too intrusive and risky to be made as an automatic feature. How about adding an --arch global option to mx to force the arch value selected?

@graalvmbot
Copy link
Collaborator

Ergys Dona has signed the Oracle Contributor Agreement (based on email address ergys -(dot)- dona -(at)- epfl -(dot)- ch) so can contribute to this repository.

@errikos
Copy link
Contributor Author

errikos commented Oct 4, 2021

  • Added a global --arch option.
  • Added a warning in case the specified architecture is different than the one detected by get_arch().
  • Introduced a new function called get_effective_arch() to get the architecture that will be effectively used in operation. This is to keep the get_arch() semantics unmodified.
  • Replaced the get_arch() calls with get_effective_arch().

There are currently no restrictions in the architecture that can be specified, except from the list of allowed options.

@dougxc
Copy link
Member

dougxc commented Oct 4, 2021

We don't want to introduce a new concept of "effective arch" and now all callers now need to think about the diff between get_arch and get_effective_arch. Can you think of a scenario under which 2 different answers make sense in a single mx execution?

@errikos
Copy link
Contributor Author

errikos commented Oct 4, 2021

We don't want to introduce a new concept of "effective arch" and now all callers now need to think about the diff between get_arch and get_effective_arch. Can you think of a scenario under which 2 different answers make sense in a single mx execution?

To be honest the only reason for this change is to display the real underlying architecture in the introduced warning message.

We could instead just introduce an "internal" method _get_real_arch and leave the callers as they were.

@dougxc
Copy link
Member

dougxc commented Oct 4, 2021

That looks much better.
@gilles-duboscq does this look good to you?

mx.py Outdated Show resolved Hide resolved
@gilles-duboscq
Copy link
Member

LGTM

mx.py Outdated Show resolved Hide resolved
.metals/metals.lock.db Outdated Show resolved Hide resolved
mx.py Outdated
@@ -581,6 +581,7 @@ def __init__(self, parents=None):
"projects and store it in the given <file>. If <file> is 'default', the compilation database will "
"be stored in the parent directory of the repository containing the primary suite. This option "
"can also be configured using the MX_COMPDB environment variable. Use --compdb none to disable.")
self.add_argument('--arch', action='store', dest='arch', help='force use the specified architecture')
Copy link
Member

Choose a reason for hiding this comment

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

force use the -> force use of the

@dougxc
Copy link
Member

dougxc commented Oct 7, 2021

This PR now looks good to merge. Can you please rebase it on master and squash it to a single commit.

@errikos errikos force-pushed the arm64-macos-rosetta2 branch 2 times, most recently from 737eb08 to 5580cfd Compare October 7, 2021 15:14
@errikos errikos changed the title Support arm64 macOS via Rosetta2 Add --arch global option to support arm64 macOS via Rosetta2 Oct 8, 2021
Allows to specify a custom architecture target.
Useful for arm64-based Apple computers, until
all used native libraries are released for arm64.

Example usage:
  mx --arch amd64 ...
@graalvmbot graalvmbot merged commit cdc91e8 into graalvm:master Oct 15, 2021
@errikos errikos deleted the arm64-macos-rosetta2 branch October 16, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants