classification
Title: Rewrite getpath.c in Python
Type: enhancement Stage: patch review
Components: Interpreter Core Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: FFY00, eric.snow, ncoghlan, steve.dower, vstinner
Priority: normal Keywords: patch

Created on 2021-10-22 23:26 by steve.dower, last changed 2021-11-12 01:35 by eric.snow.

Pull Requests
URL Status Linked Edit
PR 29041 open steve.dower, 2021-10-22 23:27
Messages (11)
msg404841 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-10-22 23:26
As discussed in issue42260, combining the two getpath implementations into a single Python implementation would make it more maintainable and modifiable (particularly where distros need to patch to support alternative layouts).
msg404843 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-10-22 23:34
The PR has more work to do, but the overall layout/changes are more or less there, so happy to discuss feedback/etc.

Obviously there are a lot of edge cases here, but they seem to be mostly tested already. And I think we're early enough in alpha to find any major issues (or absorb any necessary minor changes - seems like trailing slashes might change on some paths).

There are also some changes/hacks into the new frozen module support, so that I can freeze getpath.py without turning it into a module. I really just want to execute the bytecode - no reason for any of its contents to stick around - and this works out pretty neatly. But if the changes to frozen modules seem off then maybe we can split it into totally separate freezing support?
msg405348 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-10-29 21:27
So I think I've found my first completely unavoidable API break: PyConfig_Read(config) has to work before initialisation, but is also supposed to fill out all the fields (including the search path). But because we need at least an interpreter state, we now can't calculate everything.

The only test that seems to be affected here is test_embed.test_init_read_set(), which does a PyConfig_Read() and then inserts new paths into module_search_paths before initialising. With that one skipped, I think everything else can be handled.
msg405640 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-11-03 19:21
Last remaining test failure is one that I can't figure out on my own - the freeze test is rerunning a CPython build (on Linux) and is apparently building getpath.c with the ".c.o" rule rather than the "Modules/getpath.o" rule.

Any tips as to what I should be looking at to figure this one out?
msg405641 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-11-03 20:15
On Wed, Nov 3, 2021 at 1:21 PM Steve Dower <report@bugs.python.org> wrote:
> Last remaining test failure is one that I can't figure out on my own - the freeze test is rerunning a CPython build (on Linux) and is apparently building getpath.c with the ".c.o" rule rather than the "Modules/getpath.o" rule.
>
> Any tips as to what I should be looking at to figure this one out?

That test does an out-of-tree build.  Might that be related?
msg405653 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-11-03 22:25
I'm betting the out-of-tree (actually just deeper within the same tree) bit is related, but I just can't see how. Modules/getbuildinfo.c takes extra parameters and they seem to be being used, so I can't tell why getpath.c's are not (those rules are listed right next to each other, but well above the .c.o rule).
msg405658 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-11-04 00:24
Unsurprisingly, it was a bad edit that I made to the Makefile myself. The commit that undoes it is https://github.com/python/cpython/pull/29041/commits/aedebcc45a638f5cf65d17046ae09b5cac97cebf but since I made the initial change as part of this PR, it was never merged in.

Now to find out why the old getpath could somehow locate the stdlib but new getpath cannot... (I'm guessing it is finding the "original" stdlib rather than the fresh clone, since AFAICT there's no reference at all to the original source dir)
msg405731 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-11-04 15:58
On Wed, Nov 3, 2021 at 6:25 PM Steve Dower <report@bugs.python.org> wrote:
> Now to find out why the old getpath could somehow locate the stdlib but new getpath cannot... (I'm guessing it is finding the "original" stdlib rather than the fresh clone, since AFAICT there's no reference at all to the original source dir)

What fresh clone do you mean?  test_embed is failing, not test_freeze.
So there is no out-of-tree build involved.
msg405742 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-11-04 18:05
> What fresh clone do you mean?  test_embed is failing, not test_freeze.

test_freeze is passing, but it shouldn't be able to locate a valid Lib/ directory to load modules from. So it's somehow managing to do it against the "official" logic (none of the searches are going to find `root/python-build/Lib` starting from `root/python-installation/python`).

I haven't dug into it yet, but I suspect if the root used for this test is moved outside of the main tree then it will fail again.
msg406183 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-11-12 01:26
I'm expecting another dumb error (on my part) or two in the PR, 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.
msg406184 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-11-12 01:35
On Thu, Nov 11, 2021 at 6:27 PM Steve Dower <report@bugs.python.org> wrote:
> rather than streamline anything by changing it (yet). We can do those once we know we've got something working.

+1
History
Date User Action Args
2021-11-12 01:35:38eric.snowsetmessages: + msg406184
2021-11-12 01:26:43steve.dowersetmessages: + msg406183
2021-11-04 18:05:08steve.dowersetmessages: + msg405742
2021-11-04 15:58:05eric.snowsetmessages: + msg405731
2021-11-04 00:24:59steve.dowersetmessages: + msg405658
2021-11-03 22:25:35steve.dowersetmessages: + msg405653
2021-11-03 20:15:04eric.snowsetmessages: + msg405641
2021-11-03 19:21:29steve.dowersetmessages: + msg405640
2021-10-29 21:27:28steve.dowersetmessages: + msg405348
2021-10-23 12:25:37FFY00setnosy: + FFY00
2021-10-22 23:34:33steve.dowersetmessages: + msg404843
2021-10-22 23:27:07steve.dowersetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request27452
2021-10-22 23:26:55steve.dowercreate