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

Infer python version from Makefile.envs in PyO3 #19

Merged
merged 16 commits into from
Aug 24, 2024

Conversation

ryanking13
Copy link
Member

@ryanking13 ryanking13 commented Aug 14, 2024

Instead of hardcoding the Python version, use PYO3_CROSS_PYTHON_VERSION env variable, which PyO3 uses to find the correct Python include/lib paths.

Also removed pyo3_config.ini file. Otherwise, the config file always take precedence.

@ryanking13
Copy link
Member Author

It isn't working, but at least I found that integration tests are useful enough to detect this kind or errors :) Let's see if I can fix it.

@ryanking13 ryanking13 marked this pull request as ready for review August 20, 2024 10:39
@ryanking13 ryanking13 changed the title [Draft] Infer python version from Makefile.envs in PyO3 Infer python version from Makefile.envs in PyO3 Aug 20, 2024
@ryanking13
Copy link
Member Author

It is working now. I also checked it in pyodide/pyodide in my fork ryanking13/pyodide@be7e588

@@ -213,8 +212,9 @@ def to_env(self) -> dict[str, str]:
"cxxflags": "$(CXXFLAGS_BASE)",
"ldflags": "$(LDFLAGS_BASE) -s SIDE_MODULE=1",
# Rust-specific configuration
"pyo3_cross_lib_dir": "$(CPYTHONINSTALL)/lib",
"pyo3_cross_lib_dir": "$(CPYTHONINSTALL)/sysconfigdata", # FIXME: pyodide xbuildenv stores sysconfigdata here
Copy link
Member

@hoodmane hoodmane Aug 20, 2024

Choose a reason for hiding this comment

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

What about $(CPYTHONINSTALL)/lib/python$(PYMAJOR).$(PYMINOR)/?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have that directory in our xbuildenv now. Normally sysconfigdata would be placed in that directory but we don't vendor lib/python3.X.Y/... part in the xbuildenv and only store sysconfigdata in the top-level directory.

xbuildenv/pyodide-root$ ls -R | grep ":$" | sed -e 's/:$//' -e 's/[^-][^\/]*\//--/g' -e 's/^/ /' -e 's/-/|/'
 .
 |-cpython
 |---installs
 |-----python-3.12.1 (this is CPYTHONINSTALL)
 |-------include
 |---------python3.12
 |-----------cpython
 |-----------internal
 |-------sysconfigdata
 |---------__pycache__

Maybe we can change the path in the xbuildenv, but for now the sysconfigdata directory works like a directory where first-party libraries are stored.

Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Thanks!

@ryanking13
Copy link
Member Author

Thanks for the review!

@ryanking13 ryanking13 merged commit f3f6b21 into pyodide:main Aug 24, 2024
6 checks passed
@ryanking13 ryanking13 deleted the infer-python-version-pyo3 branch August 24, 2024 04:06
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