Skip to content

Modify the new 3.10.6 version of platform.py to work with platform.rs #4135

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

Merged
merged 2 commits into from
Sep 4, 2022

Conversation

ecpost
Copy link
Contributor

@ecpost ecpost commented Aug 25, 2022

This addresses #4111

@youknowone This is the original change I had planned to do. See my 3rd comment here regarding your try/except comment. Let me know if you'd like me to do something like that instead. (Although I'm not sure how the import of _platform would ever fail without other things falling apart first.)

@youknowone
Copy link
Member

import _platform never fails. But if we delete code, it will be unconciously recovered whenever we update platform.py from recent CPython version. deleted code is not easy to track.
I'd like to keep the original code as much as possible not to mislead library updating process.

I now think try: import _platform is not necessary. Instead of it, from _platform import * at the bottom of the platform.py will work by overriding the python attributes.

@ecpost
Copy link
Contributor Author

ecpost commented Aug 26, 2022

Funny, I was thinking that too, earlier, and gave it a try. (It worked fine.) But I figured there was a reason/preference for not doing it that way in the earlier versions. Anyway, I'll go ahead with that.

(This will also take care of those tests that failed, that I forgot to check first -- oops! I'm curious though how the pre-3.10.6 version got through with the same issue. There's a cache variable that was removed, that the test tries to clear.)

BTW, there are those two TODO items in the prior version that short-circuited a couple things. Let me take a closer look at those to see if they're important. I'll submit the change after I check those.

@youknowone
Copy link
Member

youknowone commented Aug 26, 2022

But I figured there was a reason/preference for not doing it that way in the earlier versions.

There could be an historical issue we couldn't do. The change made long time ago and we had a huge progress on compatibility. Maybe we just couldn't make the best decision for every time.

@ecpost
Copy link
Contributor Author

ecpost commented Aug 26, 2022

I confirmed, the old TODO's weren't necessary.

However... The test now fails because they're doing some "interesting" things in test_platform.py, setting sys.version and sys.platform to fake out the platform.* functions. We could probably support this in platform.rs (have it check sys.version and sys.platform). But what they're doing isn't really a documented feature, and the rust code will get pretty convoluted. But what do you think? Should I just modify the test to handle our rust overrides better? (That link you sent earlier about updating tests was very informative. Thanks.)

(I have other ideas that would involve changes to platform.py. But since we're trying to minimize changes there...)

@ecpost
Copy link
Contributor Author

ecpost commented Aug 27, 2022

Latest commit assumes that the simplified approach (just the import at the end) and an expectedFailure on that specific test is fine. Happy to make adjustments if necessary.

@youknowone
Copy link
Member

I am really sorry, but after reviewing the failing test, I think _platform was introduced only for historical reason and now we are ready to leave it to past. This is not what I expected when I first reviewed this issue.
I had a busy week but now I can spend more time for RustPython. If you want to look for an issue to step into RustPython, I will try to find one. Please leave a comment.

@youknowone youknowone merged commit fe10a4b into RustPython:main Sep 4, 2022
@ecpost
Copy link
Contributor Author

ecpost commented Sep 4, 2022

Understood, no problem. (Regarding other issues to work on, I was going to do one of the number protocol items when I get a chance later.)

@ecpost ecpost deleted the platform-from-rust branch September 25, 2024 18:32
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.

2 participants