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

Support Windows CPython interpreters distributed by non-standard orgs #2504

Merged
merged 24 commits into from
Feb 27, 2023
Merged

Support Windows CPython interpreters distributed by non-standard orgs #2504

merged 24 commits into from
Feb 27, 2023

Conversation

faph
Copy link
Contributor

@faph faph commented Feb 14, 2023

See issue #2491

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

Window Registry does not contain implementation (e.g. CPython) information
@faph
Copy link
Contributor Author

faph commented Feb 14, 2023

Testing here a straightforward fix whereby the Windows Registry-based pre-filtering is removed altogether. I assume this pre-filtering is there for performance reasons?

The issue with the Window Registry-based pre-filtering is that implementation meta data is not available. Previously a hack was implemented to map organizations "PythonCore" and "Continuum" to the "CPython" implementation. That obviously does not work for anyone else!

In any case, the existing logic applied full matching based on meta-data provided by the interpreter executable itself. That logic is cached in any case, so normally we expect this to be evaluated once per interpreter. That seems to suggest the Windows Registry-based optimization is largely redundant?

This would also bring it in line with the Linux implementation which simply relies on interpreter paths on the PATH OS environment variable. Again, that would delegate the spec matching fully to the interpreter executable itself.

@faph
Copy link
Contributor Author

faph commented Feb 14, 2023

Local test runs seem to suggest existing tests are fully passing with this change.

@gaborbernat Are you able to approve the GH Actions workflow to confirm the tests are green?

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Test and changelog needs to be added.

@faph
Copy link
Contributor Author

faph commented Feb 14, 2023

Understood - have some tests replicating the issue that I need to tidy up first.

Just looking for guidance whether this is at all a solution you're willing to consider @gaborbernat

@faph
Copy link
Contributor Author

faph commented Feb 15, 2023

For info, I am still trying to find a good testing strategy that covers the Pep514PythonInfo.from_exe(...) logic.

@faph faph changed the title [WIP] Support Windows CPython interpreters distributed by non-standard orgs Support Windows CPython interpreters distributed by non-standard orgs Feb 15, 2023
@faph faph marked this pull request as ready for review February 15, 2023 14:55
@faph
Copy link
Contributor Author

faph commented Feb 15, 2023

@gaborbernat This should be a complete PR, if you are able to approve the workflows and review, that would be great.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Loooks ok in general 👍

tests/unit/discovery/windows/conftest.py Outdated Show resolved Hide resolved
tests/unit/discovery/windows/conftest.py Outdated Show resolved Hide resolved
tests/unit/discovery/windows/conftest.py Outdated Show resolved Hide resolved
tests/unit/discovery/windows/conftest.py Outdated Show resolved Hide resolved
tests/unit/discovery/windows/conftest.py Outdated Show resolved Hide resolved
tests/unit/discovery/windows/conftest.py Outdated Show resolved Hide resolved
tests/unit/discovery/windows/test_windows.py Show resolved Hide resolved
@faph
Copy link
Contributor Author

faph commented Feb 16, 2023

@gaborbernat I think that's all feedback addressed. Thanks!

@faph
Copy link
Contributor Author

faph commented Feb 26, 2023

Appreciate your status atm @gaborbernat. Any indication when we might be able to merge and release this?

@gaborbernat gaborbernat merged commit 5a53614 into pypa:main Feb 27, 2023
@faph faph deleted the feature/win-reg-org-name branch February 28, 2023 16:44
@MeCorey
Copy link

MeCorey commented Mar 4, 2023

beautiful!

tarpas pushed a commit to tarpas/virtualenv that referenced this pull request Jun 8, 2023
…pypa#2504)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

3 participants