Skip to content
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

sys._home -> sys.base_exec_prefix #42

Merged
merged 1 commit into from
Dec 12, 2021
Merged

sys._home -> sys.base_exec_prefix #42

merged 1 commit into from
Dec 12, 2021

Conversation

imba-tjd
Copy link
Contributor

@imba-tjd imba-tjd commented May 17, 2021

Facts:

Problems:

  • When using MS Store version of Python, include sys._home makes no sense. It's like C:\Users\<user>\AppData\Local\Microsoft\WindowsApps\PythonSoftwareFoundation.Python.3.9_qbz5n2kfra8p0 and only contains 0KB files
    图片
  • What's the point to include the folder only when in venv?

Changes:

  • Always include base_exec_prefix

Other options:

  • include it still only when in venv
  • remove it, because right now it's not included outside of venv

Note:

  • I don't know whether to choose sys.base_exec_prefix or sys.base_prefix, though they are identical in my environment.

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

* What's the point to include the folder only when in venv?

I'm afraid I don't have any expertise on the reported concern. The comment says "Append the source distribution library directories, [allowing] distutils on windows to work in the source tree".

I also observe that {base_exec_prefix}/libs is appended earlier, referencing bpo-16116.

It does seem surprising to me that sys.base_exec_prefix should be added to the library_dirs with slight variations (one with libs and the other without) and under different conditionals (!= sys.prefix or unconditionally).

I don't know how sys._home is used or why library_dirs attempts to honor it here, but I don't think it's safe to twiddle the value based on a few select use-cases.

* I don't know whether to choose `sys.base_exec_prefix` or `sys.base_prefix`, though they are identical in my environment.

I don't see anything in the description above about why you would want to add either of these values. What behavior is enabled by choosing one of these prefixes (what behavior fails without this change)?

Here's what I'd like to see to make a change like this:

  • As best as possible, trace (through source history tracing) the origin of this code and attempt to replicate the environment for which this code was created. Determine if that motivation is still present, and if so, create a test to capture that requirement.
  • Capture the desired behavior that this PR is attempting to create and create a test for that, something that fails before the patch and passes after it.

Unfortunately, distutils is weakly covered and tested but also used by a wide array of environments and configurations, so any changes we make will need to be carefully considered and start to build toward a more robust and rigorously specified implementation.

I'm happy to work with you on this effort, but I doubt it will be a quick fix.

@imba-tjd
Copy link
Contributor Author

imba-tjd commented Jul 4, 2021

I don't have much experience either.

The reason I found this is because there is a VCRUNTIME140.dll under sys.base_exec_prefix, but not under _sys_home when using MS Store Python.

If both my two PRs are merged, there won't be a ld error as I said in #41 and people can successfully use MinGW to compile extensions.
Without this PR but merge #41, there would be a strange situation, that only when in venv and not using Store Python can build_ext with MinGW.

In linux, _sys_home is /usr/bin, sys.base_exec_prefix and sys.base_prefix are /usr. So they are not the same.
In my tested virtual machine Windows, _sys_home and sys.base_exec_prefix are the same, which is C:\Users\IEUser\AppData\Local\Programs\Python\Python38-32
I think _sys_home represents where the real python39.exe located.

If so, the minimal change is changing self.library_dirs.append(_sys_home) to self.library_dirs.append(sys.base_exec_prefix). Keep if _sys_home so that it still only takes effects in venv.

Though I don't know if it's appropriate to use VCRUNTIME140.dll that is bundled with Python.
But that's not very important, because _sys_home is included anyway without this patch.

@imba-tjd imba-tjd marked this pull request as draft July 7, 2021 13:27
@jaraco
Copy link
Member

jaraco commented Dec 11, 2021

May I recommend to revisit this issue and file a report that describes the issue. In particular, what do you expect to happen and what happens instead and under what conditions? And then the next step will be to create two tests as mentioned above:

  • One test to capture the current behavior (whatever motivated the author to add sys._home to the path). That is, not specifically that sys._home is added, but whatever behavior would break if it's missing.
  • Another test to capture the expectation that's missed if base_exec_prefix is not added.

These two tests may be the same thing.

@imba-tjd
Copy link
Contributor Author

I really don't have time for programming now. 😢
In the future I maybe firstly learn how to use linux and how to use python on linux. Then try to understand what sys._home represents.

@jaraco jaraco marked this pull request as ready for review December 12, 2021 14:46
@jaraco
Copy link
Member

jaraco commented Dec 12, 2021

On further consideration, and after reviewing the patch in bpo-15367 (7e4558c), it may be asking a lot to capture the expectation that this change introduces. I also agree, that the sys._home was probably a hack and superseded by sys.base_exec_prefix, which was only introduced in Python 3.3. This newer code is cleaner and likely has the intended effect. And since the failure mode is only in MS Store Python, it's probably not easy to test for that environment.

I did read up on the difference between sys.base_exec_prefix and sys.base_prefix. Basically the difference is the same as for sys.exec_prefix and sys.prefix, which is that without exec_, it's for platform independent files. So I'm comfortable that base_exec_prefix is the best choice here.

Let's incorporate this change and see how it goes. Thanks for the contrib.

@jaraco jaraco merged commit bb74c98 into pypa:main Dec 12, 2021
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