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

Fix _32_BIT_INTERPRETER for GraalPy #614

Closed
wants to merge 2 commits into from
Closed

Conversation

isuruf
Copy link

@isuruf isuruf commented Nov 12, 2022

On GraalPy's 64-bit interpreter, sys.maxsize is 2**31-1 because Java arrays can be indexed only with 32-bit indices.

cc @timfel

TODO:

  • keep backwards compat
  • check macos

On GraalPy's 64-bit interpreter, `sys.maxsize` is `2**31-1` because
Java arrays can be indexed only with 32-bit indices.
@nanonyme
Copy link

What I would suggest is creating new API so Python interpreter could itself declare whether it's 32bit or 64bit without needing these hacks. This problem has now resurfaced over and over again for many many years.

@nanonyme
Copy link

Also has this been tested to work with macOS universal binaries? See https://docs.python.org/3/library/platform.html

@isuruf isuruf marked this pull request as draft November 12, 2022 21:03
@timfel
Copy link

timfel commented Nov 14, 2022

What I would suggest is creating new API so Python interpreter could itself declare whether it's 32bit or 64bit without needing these hacks. This problem has now resurfaced over and over again for many many years.

Unless we actually go for a new API, it may be easier (and more backwards compatible) to just test if we're on Java right now, because afaik this issue of wrong detection only affects graalpy (and jython3) right now? (Not sure about the state of Jython3). Both could be tested for by trying to import java, if such arguably hacky testing is acceptable.

@nanonyme
Copy link

nanonyme commented Nov 14, 2022

To be fair, the entire sys.maxint comparison is a macOS universal binary specific hack. On other platforms you can just platform.architecture()[0] == "32bit". The "perhaps other platforms" comment in standard library documentation is purely hypothetical.

@nanonyme
Copy link

So it might potentially make more sense to detect Apple and use the special sys.maxint solution there.

@brettcannon
Copy link
Member

What I would suggest is creating new API so Python interpreter could itself declare whether it's 32bit or 64bit without needing these hacks.

That's a much bigger ask as that requires upstream Python to agree to that. That also doesn't solve things for everyone between now and some new API being added to declare if an interpreter is 32-bit or 64-bit.

@isuruf any reason this is in draft?

@nanonyme
Copy link

What I would suggest is creating new API so Python interpreter could itself declare whether it's 32bit or 64bit without needing these hacks.

That's a much bigger ask as that requires upstream Python to agree to that. That also doesn't solve things for everyone between now and some new API being added to declare if an interpreter is 32-bit or 64-bit.

@isuruf any reason this is in draft?

As said, there actually was API for this already introduced in Python 2 but it's not compatible with macOS universal binaries which are both 32bit and 64bit at the same time. I'm not sure if the use case on macOS is valid anymore. Is there such a thing as 32bit macOS?

@@ -37,7 +37,7 @@
}


_32_BIT_INTERPRETER = sys.maxsize <= 2**32
_32_BIT_INTERPRETER = (struct.calcsize("P") == 4)

Choose a reason for hiding this comment

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

I don't get this. Isn't 2**31-1 <= 2**32 ?

Choose a reason for hiding this comment

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

Iirc the point is if sys.maxint is larger than 2**32, int must be 64bit.

@brettcannon
Copy link
Member

Is there such a thing as 32bit macOS?

For modern macOS, no, but yes for older versions; macOS Mojave 10.14 was the last version to support 32-bit apps and it's only 4 years old.

@brettcannon
Copy link
Member

Closing this PR as it's been more than 6 months and it's still draft with no new commits made.

@timfel
Copy link

timfel commented Aug 15, 2023

Since this is coming up again for manylinux and also with maturin: @brettcannon you seemed concerned here about 32-bit macos support, citing Mojave. At the time of your comment, Mojave was "only" 1 year EOL. Is there a policy when support for older macOS releases is dropped? Then I would like to revive this change and maybe simply use platform.architecture()[0] == "32bit" going forward?

@brettcannon
Copy link
Member

@timfel As a project, I don't think packaging has had a discussion around this explicitly. But in general, we want to support all supported versions of Python, and if those versions can run on the older macOS versions that are still being used, it makes dropping support a bit harder (remember, us dropping it means pip drops it).

Could you open an issue to have that discussion?

@brettcannon
Copy link
Member

FYI https://www.python.org/downloads/release/python-380/ says Python 3.8.0 supports macOS 10.9 . For https://www.python.org/downloads/release/python-370/ , it goes back to 10.6 for 32-bit builds (which this project still supports until probably after our next release).

@brettcannon
Copy link
Member

And the reason I looked this up because conda-forge changed their support for macOS to 10.13.

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.

6 participants