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.

Sign up for GitHub

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

Jump to bottom

[python3] add vcpkg_get_vcpkg_installed_python #38929

Merged
merged 11 commits into from
Jun 17, 2024
Merged

[python3] add vcpkg_get_vcpkg_installed_python #38929

merged 11 commits into from
Jun 17, 2024

Conversation

Neumann-A
Copy link
Contributor

alternative to #38297

@MonicaLiu0311 MonicaLiu0311 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 27, 2024
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label May 28, 2024
@MonicaLiu0311 MonicaLiu0311 removed the info:reviewed Pull Request changes follow basic guidelines label May 29, 2024
MonicaLiu0311
MonicaLiu0311 previously approved these changes May 31, 2024
View reviewed changes
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label May 31, 2024
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 3, 2024
@vicroms
Copy link
Member

vicroms commented Jun 7, 2024

@BillyONeal
@data-queue
@JavierMatosD
@ras0219-msft
@vicroms

We discussed this PR during our meeting and have the following comments to make:

  • We think this is a good alternative to [python3] Add sitecustomize on windows for dll lookup #38297
  • This resolves Billy's security concerns because it doesn't add extra DLL loading for all people deploying Python bits.
  • What do you think about turning this into a separate helper port, for example, python3-interp with a "supports": "native" clause. That could also allow users to inject a system Python more easily.

@vicroms vicroms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 7, 2024
@Neumann-A Neumann-A force-pushed the get_working_installed_python branch from 52e01b7 to 2b3d1e8 Compare June 8, 2024 06:55
vicroms
vicroms reviewed Jun 12, 2024
View reviewed changes
ports/vcpkg-get-python/vcpkg-port-config.cmake Outdated
return()
endif()

if(PORT STREQUAL "python3") # Just for manual testing
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved the required logic for testing into the python3 port.

@BillyONeal BillyONeal changed the title [python3] add get_working_installed_python [python3] add vcpkg_get_vcpkg_installed_python Jun 13, 2024
@MonicaLiu0311 MonicaLiu0311 removed the info:reviewed Pull Request changes follow basic guidelines label Jun 13, 2024
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jun 17, 2024
@BillyONeal BillyONeal merged commit 2324733 into microsoft:master Jun 17, 2024
17 checks passed
@BillyONeal
Copy link
Member

BillyONeal commented Jun 17, 2024 •
edited
Loading

Thanks for the new feature!

@Neumann-A Neumann-A deleted the get_working_installed_python branch June 20, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vicroms vicroms vicroms left review comments

@dg0yt dg0yt dg0yt left review comments

@MonicaLiu0311 MonicaLiu0311 MonicaLiu0311 approved these changes

Assignees

@MonicaLiu0311 MonicaLiu0311

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Neumann-A @vicroms @BillyONeal @dg0yt @MonicaLiu0311