Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-102536: Added_architecture
field tosys.implementation
#124582
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to have tests as well please.
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Library/2024-09-26-14-43-32.gh-issue-102536.xLiIvi.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Okay, I have make change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry but this requires more investigation. The value being added is not the correct one (sys.platform
is already exposed so it does not make sense to havesys.implementation._architecture
being the same IMO).
Uh oh!
There was an error while loading.Please reload this page.
rruuaanng commentedSep 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
According to |
Yes but the attribute being added is not the one that you added. You need to get the one stored in |
rruuaanng commentedSep 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hmmm, I know how to get the architecture information in capi, but it seems that this is only supported on unix-like. |
picnixz commentedSep 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
On the issue:
So we need to know how to recover cpython/PC/layout/support/props.py Line 11 inabe5f79
I'll leave it to@zooba to explain exactly what he had in mind. But AFAICT, the value of |
He probably needs a descriptive field for the current system architecture in the implementation (since ArchName is limited to this purpose). I think that's what he had in mind, so my PR applies to all targets that support the |
Yeah, this is compile-time information that isn't accessible at runtime (remembering that Windows supports CPU emulation, so an x64 machine can run a 32-bit build, and an ARM64 machine can run an x86 or x64 build). As for how to get it at compile time, look in |
rruuaanng commentedSep 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In other words, what is needed is the
Don't this
|
rruuaanng commentedSep 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I found this in python.prop
If what you mentioned by
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The issue is called "Issue: extend sys module on windows" but your implementation is only for non-Windows platforms.
Uh oh!
There was an error while loading.Please reload this page.
In case it got lost in the shuffle, I want to reiterate that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this supposed to be a public interface (just prefixed with_
to denote platform incompatibility), or a private implementation detail that we need internally? If it's the former, then I agree with Eric.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Does it belong in |
Even if we make it available in |
rruuaanng commentedOct 16, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Your idea is that if he is not in sysconfig, then we need it and he also provides it in sysconfig. Edit If I understand your idea correctly, then this question should open a new PR, and I can make him support it. |
Why does this test fail? |
@rruuaanng: Please follow what@erlend-aasland said:
|
I close the issue for now, I suggest to discuss on the issue and/or on thehttps://discuss.python.org/t/regarding-whether-we-should-add-py-currentarch-or-py-archname-function/65191 discussion. |
I think so. If there are concrete results in the community discussion, maybe I can reopen this PR (it was closed by someone else, so I can't reopen it). |
No reason to close the PR yet. |
Thanks for zooba support. I agree. In fact, this issue has been discussed. And it has met all the requirements of the merger. I really like this function. It seems that only this can get the information at the time of build. |
@zooba I'm curious about the reason to put this in |
zooba comment from this PR of issue.
|
@ZeroIntensity My rationale is on the issue:#102536 (comment) |
Doc/library/sys.rst Outdated
@@ -1122,6 +1122,12 @@ always available. | |||
``cache_tag`` is set to ``None``, it indicates that module caching should | |||
be disabled. | |||
*arch* describes the architecture of the current interpreter runtime system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"current interpreter runtime system" is wrong--_architecture
defines what architecture CPython was built with, not what it's currently running under.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's close. It specifies the architecture it was builtto run as, which is independent from both the one it was builton, and the one it's currentlyrunning on.
But it's the only one that is specific to the currently running interpreter. So at most, I'd only change this to "the native architecture of the current interpreter", and possibly add "which will usually match the architecture of the current system" (though that's probably more words than it's worth).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cc@zooba
sys
module on windows #102536