Skip to content

Conversation

@zklaus
Copy link
Owner

@zklaus zklaus commented Mar 14, 2025

This adds sys.abi_features.

@zklaus zklaus marked this pull request as ready for review March 14, 2025 19:30
Copy link

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for providing this, along with the rest of the implementation stuff. I left a few comments inline.

@zklaus zklaus requested a review from lysnikolaou March 18, 2025 16:23
Copy link

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

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

Looks great! Just three very minor comments.

zklaus and others added 2 commits March 18, 2025 18:47
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
@zklaus zklaus requested a review from lysnikolaou March 18, 2025 17:49
Copy link

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

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

Looks good! We probably shouldn't merge it though. It's best to keep it in a separate branch.

@zklaus
Copy link
Owner Author

zklaus commented Mar 18, 2025

Agreed. Thanks for the reviewing! 🙏

Comment on lines +27 to +29
``32-bit`` or ``64-bit``
The bitness of the interpreter, that is, whether it is a 32-bit or 64-bit
build.
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

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.

@FFY00
Copy link

FFY00 commented Mar 28, 2025

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),
}

@AA-Turner
Copy link

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.

@FFY00
Copy link

FFY00 commented Mar 28, 2025

Would the flag metadata also apply to Windows? I think this comment is relevant.

Yes, it's just that debug is special. Folks likely shouldn't be constructing the native module extension in most cases anyway.

The frozenset design seems more straightforward to adapt into an environment marker than a dictionary.

How so? frozenset(sys.abi_features) gives you the exact same result.

@AA-Turner
Copy link

How so? frozenset(sys.abi_features) gives you the exact same result.

Ah, I had thought that your proposal was for the keys to be the feature, mapping to the value (i.e. {'bitness': Feature(...)}. If it's just expanding the proposal in the PEP to add more metadata, I have no objection (but I'd advocate for a frozen type rather than a plain dict).

@FFY00
Copy link

FFY00 commented Mar 28, 2025

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.

Comment on lines +3518 to +3528
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;
}
Copy link

@XuehaiPan XuehaiPan Mar 31, 2025

Choose a reason for hiding this comment

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

Would 0x7FFFFFFFFFFFFFFFL overflow on some platforms?

Suggested change
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>
@zklaus
Copy link
Owner Author

zklaus commented Apr 4, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants