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

gh-127503 Don't propagate native PATH to Emscripten Python #127633

Merged

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Dec 5, 2024

The native PATH makes shutil.which() pick up native executables which is inconvenient.

While I'm at it, I also set thisProgram incorrectly in the final version of #127506.

This makes `shutil.which()` pick up native executables which is inconvenient.
@freakboy3742
Copy link
Contributor

I thinks this change makes sense; however, in trying to test it, I've hit an issue.

I don't think the problem is related to this change specifically; I can't tell if it's related to #127506, or if I've done something to my EMSDK config. I suspect it's manifesting now because I had to do a complete rebuild of both the host and build Python, whereas during my testing of #127506 I had a pre-existing build, and I was only testing the script updates.

The problem manifests as an error during python Tools/wasm/emscripten make-host. I had to remove the --jobs flag to get an error that made any sense; when I do, I get:

...
_PYTHON_HOSTRUNNER='node' _PYTHON_PROJECT_BASE=/Users/rkm/projects/python/cpython/cross-build/wasm32-emscripten _PYTHON_HOST_PLATFORM=emscripten-wasm32 PYTHONPATH=../../Lib _PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata__emscripten_wasm32-emscripten _PYTHON_SYSCONFIGDATA_PATH= /Users/rkm/projects/python/cpython/cross-build/build/python.exe -S -m sysconfig --generate-posix-vars ;\
        if test $? -ne 0 ; then \
                echo "generate-posix-vars failed" ; \
                rm -f ./pybuilddir.txt ; \
                exit 1 ; \
        fi
Written build/lib.emscripten-wasm32-3.14/_sysconfigdata__emscripten_wasm32-emscripten.py
Written build/lib.emscripten-wasm32-3.14/_sysconfig_vars__emscripten_wasm32-emscripten.json
_PYTHON_HOSTRUNNER='node' _PYTHON_PROJECT_BASE=/Users/rkm/projects/python/cpython/cross-build/wasm32-emscripten _PYTHON_HOST_PLATFORM=emscripten-wasm32 PYTHONPATH=../../Lib _PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata__emscripten_wasm32-emscripten _PYTHON_SYSCONFIGDATA_PATH=/Users/rkm/projects/python/cpython/cross-build/wasm32-emscripten/build/lib.emscripten-wasm32-3.14 /Users/rkm/projects/python/cpython/cross-build/build/python.exe -c 'import sys ; from sysconfig import get_platform ; print("%s-%d.%d" % (get_platform(), *sys.version_info[:2]))' >platform
/Users/rkm/projects/emsdk/upstream/emscripten/emcc  -fno-strict-overflow -Wsign-compare -Wunreachable-code -DNDEBUG -g -O3 -Wall -DPY_CALL_TRAMPOLINE -sUSE_BZIP2   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I../../Include/internal -I../../Include/internal/mimalloc -IObjects -IInclude -IPython -I. -I../../Include   -fPIC -fPIC -c ../../Modules/_testimportmultiple.c -o Modules/_testimportmultiple.o
/Users/rkm/projects/emsdk/upstream/emscripten/emcc -shared -sSIDE_MODULE=1   -sALLOW_MEMORY_GROWTH -sINITIAL_MEMORY=20971520 -sWASM_BIGINT -sFORCE_FILESYSTEM -lidbfs.js -lnodefs.js -lproxyfs.js -lworkerfs.js -sEXPORTED_RUNTIME_METHODS=FS,callMain,ENV -sEXPORTED_FUNCTIONS=_main,_Py_Version -sSTACK_SIZE=5MB -sEXIT_RUNTIME   Modules/_testimportmultiple.o   -o Modules/_testimportmultiple.cpython-314-wasm32-emscripten.so
emcc: warning: EXPORTED_FUNCTIONS is not valid with LINKABLE set (normally due to SIDE_MODULE=1/MAIN_MODULE=1) since all functions are exported this mode.  To export only a subset use SIDE_MODULE=2/MAIN_MODULE=2 [-Wunused-command-line-argument]
emcc: error: undefined exported symbol: "_Py_Version" [-Wundefined] [-Werror]
make: *** [Modules/_testimportmultiple.cpython-314-wasm32-emscripten.so] Error 1
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/Users/rkm/projects/python/cpython/Tools/wasm/emscripten/__main__.py", line 357, in <module>
    main()
  File "/Users/rkm/projects/python/cpython/Tools/wasm/emscripten/__main__.py", line 353, in main
    dispatch[context.subcommand](context)
  File "/Users/rkm/projects/python/cpython/Tools/wasm/emscripten/__main__.py", line 83, in wrapper
    return func(context, working_dir)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/rkm/projects/python/cpython/Tools/wasm/emscripten/__main__.py", line 252, in make_emscripten_python
    call(
  File "/Users/rkm/projects/python/cpython/Tools/wasm/emscripten/__main__.py", line 110, in call
    subprocess.check_call(command, **kwargs, stdout=stdout, stderr=stderr)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['make', 'all']' returned non-zero exit status 2.

Any ideas where that could be coming from? It looks like it's a misconfiguration of the -sEXPORTED_FUNCTIONS setting... but that's not a recent change, AFAICT

@hoodmane
Copy link
Contributor Author

hoodmane commented Dec 6, 2024

I think the problem was introduced in the web_example PR here:
https://github.com/python/cpython/pull/127113/files#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810R2336

The issue is that -sEXPORTED_FUNCTIONS (and the majority of the other Emscripten-specific settings) should only be added when linking python.mjs not when linking the various shared libraries. Here it's failing to link _testimportmultiple.so because it doesn't define a symbol called Py_Version. I think the solution is to put those flags into LINKFORSHARED and not LDFLAGS_NODIST. (The variable names in configure.ac can be a bit hard to understand.)

@hoodmane
Copy link
Contributor Author

hoodmane commented Dec 6, 2024

Opened #127666 with a fix.

Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍 With the fix from #127666, I can now confirm this works as expected.

@freakboy3742 freakboy3742 merged commit 5876063 into python:main Dec 9, 2024
48 checks passed
@hoodmane hoodmane deleted the emscripten-dont-propagate-native-path branch December 9, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants