Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-131876: use extern storage for _hashlib helper functions#137301
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
picnixz commented Aug 1, 2025 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as duplicate.
This comment was marked as duplicate.
picnixz commented Aug 1, 2025
!buildbot WASM Emscripten |
bedevere-bot commented Aug 1, 2025
🤖 New build scheduled with the buildbot fleet by @picnixz for commit 033ba23 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137301%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
picnixz commented Aug 1, 2025
Ok, I think the Windows build is now ok. I'm pretty sure the WASM build is still broken because I don't think |
picnixz commented Aug 1, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ok, so I see the problem. libhashlib.a is indeed included by the hashlib module and by all other HACL* components. It's the same issue as #133042 but the "hacky" fix can be as follows: I just link |
zooba commented Aug 1, 2025
I don't think The trickier part is trying to use the same C file in multiple projects, which is notoriously troublesome. The Windows build will handle it okay, because it builds the file multiple times in separate locations with separate settings (for each project that it's compiled into), but the Makefile can sometimes be a problem because it'll typically only build the source file once for all projects. Seems to be okay here, but something to be prepared for if reports start coming in that it isn't working. |
picnixz commented Aug 1, 2025
!buildbot WASM Emscripten |
bedevere-bot commented Aug 1, 2025
🤖 New build scheduled with the buildbot fleet by @picnixz for commit 561f4a9 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137301%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
picnixz commented Aug 1, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Urgh, it still doesn't work on Emscripten. Unfortunately, I can't just disable those modules... So I'll need to investigate more. |
picnixz commented Aug 1, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ok, so the issue is that the @hoodmane how can I make an internal CPython-only library that:
If this is critically hindering the development on WASM (which I believe it is as it's no more possible to compile Python I think?), I will revert my commit (unless the fix I suggest now works, idk). EDIT: My latest fix is trying to add explicitly the libs that are needed to the interpreter. |
picnixz commented Aug 1, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ok, the latest idea doesn't seem to really work (either I messed up somewhere else or it's fundamentally wrong). I'm going to revert this: #137307 because it's hindering the development of WASM. |
hoodmane commented Aug 1, 2025
Well if the archive isn't needed at all the linker will just leave it out. So you could put in Or we could try wrapping the wasm linker and dropping the duplicated library. |
picnixz commented Aug 1, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
That... would be interesting indeed.. but ideally, I'd like to have the build process for WASI/WASM more or less the same as for other platforms. It becomes very hard to make incremental changes. For now, I'll revert the changes so to avoid blocking further development. Another temporary solution is to simply put everything as a
That was my idea, but we need to expand makefile variables before because the duplication appears because of |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
picnixz commented Aug 1, 2025
Hum... apparently my approach here works... I'll cleanup those hacks tomorrow. |
picnixz commented Aug 2, 2025
Too hard to solve the conflicts, I'm going to start a fresh PR. |
_hashliband_hmac#131876