Conversation
lysnikolaou
left a comment
There was a problem hiding this comment.
Looks great! Thanks for providing this, along with the rest of the implementation stuff. I left a few comments inline.
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
lysnikolaou
left a comment
There was a problem hiding this comment.
Looks great! Just three very minor comments.
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
lysnikolaou
left a comment
There was a problem hiding this comment.
Looks good! We probably shouldn't merge it though. It's best to keep it in a separate branch.
|
Agreed. Thanks for the reviewing! 🙏 |
| ``32-bit`` or ``64-bit`` | ||
| The bitness of the interpreter, that is, whether it is a 32-bit or 64-bit | ||
| build. |
There was a problem hiding this comment.
I don't think this is the right place. This information is part of the target architecture, where this API is meant to expose features of the Python ABI — see PEP 3149.
FFY00
left a comment
There was a problem hiding this comment.
When I originally proposed this in the discussion, I had in mind an API that provided he feature name, as well as maping it to its character abbreviation.
A dictionary or some kind of custom object for future expandability should work.
|
A more future-proof design could look something like this: >>> sys.abi_features
{
'free-threading': Feature(name='free-threading', flag='t', default=False),
'debug': Feature(name='debug', flag='d', default=False),
} |
|
Would the flag metadata also apply to Windows? I think this comment is relevant. The frozenset design seems more straightforward to adapt into an environment marker than a dictionary. |
Yes, it's just that
How so? |
Ah, I had thought that your proposal was for the keys to be the feature, mapping to the value (i.e. |
|
OTOH, we should probably have an API that can map the ABI feature name to the flag character for all features, not just the ones present, so my design isn't great either. |
| switch (PY_SSIZE_T_MAX) { | ||
| case 0x7FFFFFFFL: | ||
| bitness = PyUnicode_FromString("32-bit"); | ||
| break; | ||
| case 0x7FFFFFFFFFFFFFFFL: | ||
| bitness = PyUnicode_FromString("64-bit"); | ||
| break; | ||
| default: | ||
| bitness = Py_NewRef(Py_None); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Would 0x7FFFFFFFFFFFFFFFL overflow on some platforms?
| switch (PY_SSIZE_T_MAX) { | |
| case 0x7FFFFFFFL: | |
| bitness = PyUnicode_FromString("32-bit"); | |
| break; | |
| case 0x7FFFFFFFFFFFFFFFL: | |
| bitness = PyUnicode_FromString("64-bit"); | |
| break; | |
| default: | |
| bitness = Py_NewRef(Py_None); | |
| break; | |
| } | |
| switch (SIZEOF_VOID_P) { | |
| case 4: | |
| bitness = PyUnicode_FromString("32-bit"); | |
| break; | |
| case 8: | |
| bitness = PyUnicode_FromString("64-bit"); | |
| break; | |
| default: | |
| bitness = Py_NewRef(Py_None); | |
| break; | |
| } |
Co-authored-by: Xuehai Pan <XuehaiPan@outlook.com>
|
Thanks for your comments, @FFY00! I think adding more metadata is not a problem. As I understand it, you agree that for the environment markers a frozen set of feature names is appropriate. Given that we are discussing the PEP as a packaging one, possibly making the addition to CPython without a PEP, I'd like to drive the discussion on the packaging side first, but am certainly open to an improved API on the CPython side. Does that make sense? |
This adds
sys.abi_features.