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

Access sysconfig paths through pyodide config CLI #14

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Jul 22, 2024

Description

This PR is a re-do of pyodide/pyodide#4947, which aimed to add the include directory containing the Python interpreter headers. This PR goes one step further and adds a way to access all of the paths listed under sysconfig.get_paths(). These paths are available in the default configuration values under pyodide config list and are accessible through pyodide config get <variable name>, say pyodide config get pyodide_sysconfig_include. These contain the pyodide_sysconfig prefix to prevent any potential conflicts with any of the other computed variables.

Note

Please refer to the description for the PR above for the reasoning behind this change.

Checklist

  • Tested via TestConfigManager_OutOfTree::test_default_config, so an additional test isn't needed
  • CHANGELOG entry – addressed in 57ef6f2

agriyakhetarpal added a commit to agriyakhetarpal/pyodide-build that referenced this pull request Jul 22, 2024
@agriyakhetarpal agriyakhetarpal force-pushed the feat/pyodide-cli-sysconfig-additions branch from dc53f07 to 1bb5c2d Compare July 22, 2024 21:24
@agriyakhetarpal agriyakhetarpal force-pushed the feat/pyodide-cli-sysconfig-additions branch from d218079 to 57ef6f2 Compare July 22, 2024 21:25
@agriyakhetarpal
Copy link
Member Author

Personally, I'm not a fan of the pyodide_sysconfig prefix, but I think it helps avoid us being ambiguous. Also, I think python_sysconfig would be a better prefix instead of pyodide_sysconfig – this is ready for review, @ryanking13 (I don't have permissions to request reviews on this repository)

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks @agriyakhetarpal!

pyodide_sysconfig_*

Maybe just sysconfig_*? pyodide prefix seems redundant to me.

# Load the values of sysconfig.get_paths() with a
# "pyodide_sysconfig_" prefix to differentiate them
# from other configuration variables
return {f"pyodide_sysconfig_{k}": v for k, v in sysconfig.get_paths().items()}
Copy link
Member

Choose a reason for hiding this comment

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

This would retrieve the host Python's sysconfig values, not those in the cross build env. We should parse these values from the _sysconfigdata__emscripten_wasm32-emscripten.py file in the xbuildenv.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where should I retrieve this file from the cross-build environment? I have noted that I can get the file when I do pyodide xbuildenv install, but if there's already a programmatic way to access this, I can use that instead of writing my own methods.

Copy link
Member

Choose a reason for hiding this comment

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

Here is the code we locate the sysconfig file. Since it uses the variables in the Makefile.envs, you'll need to modify it a bit to use it in the config manager to prevent cyclic import.

sysconfigdata_name = get_build_flag("SYSCONFIG_NAME")
sysconfigdata_path = (
Path(get_build_flag("TARGETINSTALLDIR"))
/ f"sysconfigdata/{sysconfigdata_name}.py"
)

I think you'll also need to read the sysconfig.py and how it retrieves the value from the sysconfig file, as we cannot directly use sysconfig module here.

# "pyodide_sysconfig_" prefix to differentiate them
# from other configuration variables
PYODIDE_CONFIGS.update(
{f"pyodide_sysconfig_{k}": v for k, v in sysconfig.get_paths().items()}
Copy link
Member

Choose a reason for hiding this comment

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

There are only eight values in sysconfig.get_paths(), so, I think it is okay to list all of them, which will be more explicit and make the code more readable.

@agriyakhetarpal agriyakhetarpal mentioned this pull request Sep 17, 2024
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