Skip to content

Conversation

@zooba
Copy link
Member

@zoobazooba commented Oct 18, 2021

@zooba
Copy link
MemberAuthor

This is veryvery WIP, but I wanted to share it now for discussion.

My plan is to set it up so that getpath isn't actually a module, but is frozen bytecode that we execute during startup. The top of the file has the expected globals, and I'll use C code to create the dict for it. Then we exec the code, and pull the results out of it.

The next step is to write tests, which can execute it directly without having to modify C code and prove correctness/etc. Because it's all possible to stub out, we can isolate it entirely from any real filesystem and simulate whatever layout/build settings we want.

It may be logical to also merge in some of site.py's functionality, and potentially sysconfig (looking at the install schemes), but those can be deferred to a later PR.

@zooba
Copy link
MemberAuthor

Ping @vstinner

@vstinner
Copy link
Member

If I understood correctly, this code doesn't remove getpath.c yet. It's a WIP.

@vstinner
Copy link
Member

For me, the most important part is to write unit tests to test that the same always produce the same . The code should be stateless, but can use the current working directory, environment variables, etc.

I wrote down all I know about the "Python Path Configuration" at: https://docs.python.org/dev/c-api/init_config.html#python-path-configuration

@zooba
Copy link
MemberAuthor

If I understood correctly, this code doesn't remove getpath.c yet.

Correct. I just started removing it, and obviously everything is broken :) But I'll come back to it tomorrow.

I wrote down all I know ...

Thanks. I think I've re-discovered most of that by parsing the logic from getpath.c, but what you've got there will help me understand how you designed the config mechanism. I want to remove as much as possible, but hopefully without breaking anyone trying to use it directly.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@ericsnowcurrently FYI, if my change doesn't make it in, you'll want to fix this check. On Windows, executable and stdlib have the same basename in a release build (e.g. C:\Python39\{Lib//python.exe}), but different in a dev build (C:\cpython\Lib // C:\cpython\PCbuild\amd64\python.exe)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This file is a complete rewrite - don't try and analyse the diff. Just go to "View File" under the triple-dot menu for it instead.

@zoobazooba changed the title bpo-42260: Port getpath[p].c to Pythonbpo-45582: Port getpath[p].c to PythonOct 22, 2021
@zoobazooba marked this pull request as ready for review October 22, 2021 23:42
@zoobazooba requested a review from a team as a code ownerOctober 22, 2021 23:42
@zooba
Copy link
MemberAuthor

This is definitely not ready to merge yet, even if by some chance all the tests pass, but I'm happy to have reviews and comments/discussion.

Any significant discussion should go to the bpo issue, please. Also, if I need to be pinged, please do it on bpo.

Copy link
Member

@ericsnowcurrentlyericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Here's a partial review. I haven't dug into the bigger pieces of this PR.

Choose a reason for hiding this comment

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

I purposefully left it -1 if we couldn't figure it out. So this change implies that we are always able to figure it out. Is that the case?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Given the rules we've got for figuring out if we're in a dev build or not, yes. Those rules may be insufficient, but it's all a heuristic anyway - if you put those files into a regular install, we'll assume it's a dev build.

Choose a reason for hiding this comment

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

we'll assume it's a dev build.

That's what I did at first too. However, I found that it was helpful to know if we were not able to figure it out, which is what the -1 indicates in this case.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I mean, we either find the build landmarks or we don't... what other checks do you have? If the build landmarks aren't there, it's not a dev build, and if they are but the stdlib isn't, it's all going to crash down anyway.

@zooba
Copy link
MemberAuthor

I'm expecting another dumb error (on my part) or two, but I'm very close to having this working.

Reviews would be appreciated! Bear in mind that I'm trying to match the current (quirky) behaviour, rather than streamline anything by changing it (yet). We can do those once we know we've got something working.

@zooba
Copy link
MemberAuthor

There was nothing helpful in that history anyway ;-)

Tests all pass now, reviews appreciated!

@zoobazooba added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zooba for commit 5962947 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2021
@zooba
Copy link
MemberAuthor

zooba commented Dec 2, 2021

Found the leak! So it's ready to merge once these tests pass.

@zoobazooba merged commit 99fcf15 into python:mainDec 3, 2021
@zoobazooba deleted the bpo-42260 branch December 3, 2021 00:08
@zoobazooba mentioned this pull request Apr 10, 2022
ales-erjavec added a commit to biolab/orange3-installers that referenced this pull request Apr 2, 2024
Problem on MacOS exec_prefix is not resolved when symlink to bin/python3.11 Probably due to python/cpython#29041
ales-erjavec added a commit to biolab/orange3-installers that referenced this pull request Apr 2, 2024
Problem on MacOS exec_prefix is not resolved when symlink to bin/python3.11 Probably due to python/cpython#29041
markotoplak pushed a commit to Quasars/quasar that referenced this pull request Jun 27, 2024
Problem on MacOS exec_prefix is not resolved when symlink to bin/python3.11 Probably due to python/cpython#29041
markotoplak pushed a commit to Quasars/quasar that referenced this pull request Jun 27, 2024
Problem on MacOS exec_prefix is not resolved when symlink to bin/python3.11 Probably due to python/cpython#29041
markotoplak pushed a commit to Quasars/quasar that referenced this pull request Jun 27, 2024
Problem on MacOS exec_prefix is not resolved when symlink to bin/python3.11 Probably due to python/cpython#29041
Sign up for freeto 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.

7 participants

@zooba@vstinner@bedevere-bot@eryksun@ericsnowcurrently@arhadthedev@the-knights-who-say-ni