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

Incorrect case string in configure script #120154

Closed
allsey87 opened this issue Jun 6, 2024 · 11 comments · Fixed by #120173
Closed

Incorrect case string in configure script #120154

allsey87 opened this issue Jun 6, 2024 · 11 comments · Fixed by #120173
Labels
build The build process and cross-build type-bug An unexpected behavior, bug, or error

Comments

@allsey87
Copy link
Contributor

allsey87 commented Jun 6, 2024

Bug report

Bug description:

I think this:

cpython/configure

Line 12895 in fd104df

Emscripten|WASI)

Should be: Emscripten*|WASI*

I could be wrong, but I think this will never be matched otherwise since even if ac_sys_release is empty, we would be trying to match Emscripten against Emscripten/

CPython versions tested on:

CPython main branch

Operating systems tested on:

Other

Linked PRs

@allsey87 allsey87 added the type-bug An unexpected behavior, bug, or error label Jun 6, 2024
@Eclips4 Eclips4 added the build The build process and cross-build label Jun 6, 2024
@allsey87
Copy link
Contributor Author

allsey87 commented Jun 6, 2024

I think the only reason why this hasn't been an issue/picked up before is that usually, for Emscripten builds, emconfigure is executed instead of configure which sets LDSHARED as an environment variable, causing the entire block to be bypassed.

@allsey87
Copy link
Contributor Author

allsey87 commented Jun 6, 2024

I can confirm that patching configure enabled me to move forwards. The values in the resulting Makefile now looks closer to what is generated in the Pyodide project with an extra -shared argument passed to emcc.

@erlend-aasland
Copy link
Contributor

Thanks for the report! It sounds like you have a patch ready; would you like to open a PR?

vstinner pushed a commit that referenced this issue Jun 7, 2024
…#120173)

Fix Emscripten/WASI pattern in case statement for LDSHARED
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 7, 2024
…SHARED (pythonGH-120173)

Fix Emscripten/WASI pattern in case statement for LDSHARED
(cherry picked from commit 47816f4)

Co-authored-by: Michael Allwright <contact@allwright.io>
erlend-aasland pushed a commit that referenced this issue Jun 7, 2024
…DSHARED (GH-120173) (#120199)

Fix Emscripten/WASI pattern in case statement for LDSHARED
(cherry picked from commit 47816f4)

Co-authored-by: Michael Allwright <contact@allwright.io>
@erlend-aasland
Copy link
Contributor

Thanks for the report and the fix, @allsey87!

@vstinner
Copy link
Member

vstinner commented Jun 7, 2024

@allsey87: Thanks for signing again the CLA with your second email address.

vstinner pushed a commit to vstinner/cpython that referenced this issue Jun 7, 2024
…SHARED (python#120173)

Fix Emscripten/WASI pattern in case statement for LDSHARED

(cherry picked from commit 47816f4)
vstinner added a commit that referenced this issue Jun 7, 2024
…DSHARED… (#120204)

gh-120154: Fix Emscripten/WASI pattern in case statement for LDSHARED (#120173)

Fix Emscripten/WASI pattern in case statement for LDSHARED

(cherry picked from commit 47816f4)

Co-authored-by: Michael Allwright <contact@allwright.io>
@encukou
Copy link
Member

encukou commented Jun 10, 2024

This broke the stable buildbot wasm32-wasi Non-Debug 3.12, see e.g. here.

@encukou encukou reopened this Jun 10, 2024
@allsey87
Copy link
Contributor Author

Are you sure it was this commit? The error that you are getting sounds unrelated to this change:

wasm-ld: error: Modules/_testimportmultiple.o: relocation R_WASM_MEMORY_ADDR_SLEB cannot be used against symbol `_testimportmultiple`; recompile with -fPIC

@vstinner
Copy link
Member

vstinner commented Jun 10, 2024

This broke the stable buildbot wasm32-wasi Non-Debug 3.12, see e.g. here.

wasm-ld: error: Modules/_testimportmultiple.o: relocation R_WASM_MEMORY_ADDR_SLEB cannot be used against symbol `_testimportmultiple`; recompile with -fPIC
wasm-ld: error: Modules/_testimportmultiple.o: relocation R_WASM_MEMORY_ADDR_SLEB cannot be used against symbol `_foomodule`; recompile with -fPIC
wasm-ld: error: Modules/_testimportmultiple.o: relocation R_WASM_MEMORY_ADDR_SLEB cannot be used against symbol `_barmodule`; recompile with -fPIC

The difference between main and 3.12 is that 3.12 tries to build test modules such as _testimportmultiple.

=> I wrote #120309 to fix the issue: on WASI, no longer build test modules on Python 3.12.

vstinner added a commit to vstinner/cpython that referenced this issue Jun 10, 2024
On WASI, test modules, such as _testimportmultiple or xxlimited, are
no longer built.
@allsey87
Copy link
Contributor Author

allsey87 commented Jun 10, 2024

@vstinner not sure if this relevant, but #120204 did change the flags for the buildbot.

Previously:

checking LDSHARED... 
checking BLDSHARED flags... 
checking CCSHARED... 
checking LINKFORSHARED... 
checking CFLAGSFORSHARED... 

Now:

checking LDSHARED... $(CC) -shared
checking BLDSHARED flags... $(CC) -shared
checking CCSHARED... 
checking LINKFORSHARED... 
checking CFLAGSFORSHARED... 

I think this is correct though, and any build configuration relying on those flags not being set would be the problem.

noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
…SHARED (python#120173)

Fix Emscripten/WASI pattern in case statement for LDSHARED
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
…SHARED (python#120173)

Fix Emscripten/WASI pattern in case statement for LDSHARED
@hugovk
Copy link
Member

hugovk commented Aug 8, 2024

Triage: the linked PRs are merged, can this be closed or is there more to do?

@allsey87
Copy link
Contributor Author

allsey87 commented Aug 8, 2024

The linked PRs are indeed merged. Closing.

@allsey87 allsey87 closed this as completed Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants