GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-131876: use `extern` storage for `_hashlib` helper functions by picnixz · Pull Request #137301 · python/cpython

Conversation

@picnixz

@picnixz picnixz commented Aug 1, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Unify recognization of message digest names for _hashlib and _hmac #131876

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@picnixz
does it work like this?
862d50a
@picnixz picnixz added the skip news label Aug 1, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Aug 1, 2025
Unify recognization of message digest names for _hashlib and _hmac #131876
Open
@picnixz

This comment was marked as resolved.

Sign in to view
@bedevere-bot

This comment was marked as duplicate.

Sign in to view
@picnixz picnixz mentioned this pull request Aug 1, 2025
gh-131876: extract _hashlib helpers into a separate directory #136995
Merged
picnixz added 2 commits August 1, 2025 14:39
@picnixz
maybe like this?
4552353
@picnixz
typo?
033ba23
@picnixz

picnixz commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

!buildbot WASM Emscripten

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Aug 1, 2025

Copy link
Copy Markdown

🤖 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: WASM Emscripten

The builders matched are:

  • WASM Emscripten PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

Ok, I think the Windows build is now ok. I'm pretty sure the WASM build is still broken because I don't think extern would change much here. For WASI, it appears that _hashlib is not even built (maybe related to #136976?). Locally it seems I can more or less build but it fails with ../../../../Python/emscripten_syscalls.c:221:7: error: use of unknown builtin '__builtin_wasm_ref_is_null_extern' [-Wimplicit-function-declaration] so it might be on my side first. Thus I'll try to get the buildbot pass.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Aug 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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 libhashlib.a to the interpreter directly as there is no OpenSSL or anything else that is needed. For now I'll only patch WASM like this. But I really want to avoid this and find a better way to handle all those cryptographic modules with different "libs".

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zooba

zooba commented Aug 1, 2025

Copy link
Copy Markdown
Member

I don't think extern is even necessary here, since that should be assumed for prototypes. Any .c that includes the internal header will resolve the functions at link time, which is the only time we want it to happen (i.e. not at dynamic load time).

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 2 commits August 1, 2025 15:44
@picnixz
directly link to the interpreter
ba5b0f3
@picnixz
regen
561f4a9
@picnixz

picnixz commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

!buildbot WASM Emscripten

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Aug 1, 2025

Copy link
Copy Markdown

🤖 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: WASM Emscripten

The builders matched are:

  • WASM Emscripten PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Aug 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Urgh, it still doesn't work on Emscripten. Unfortunately, I can't just disable those modules... So I'll need to investigate more.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 3 commits August 1, 2025 16:03
@picnixz
Revert "regen"
1e74acb
This reverts commit 561f4a9.
@picnixz
Revert "directly link to the interpreter"
6bc7d43
This reverts commit ba5b0f3.
@picnixz
explicitly define the injected libs
1ded2a3
@picnixz

picnixz commented Aug 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Ok, so the issue is that the .a is present in both LOCALMODLIBS because of MODULE__MD5_LDFLAGS and when building _md5 itself (because I need it). I don't really know what to do here. Should I just remove it from MODULE__MD5_LDFLAGS and put it in LIBPYTHON directly?

@hoodmane how can I make an internal CPython-only library that:

  • is shared across multiple modules (_hashlib and _md5 etc),
  • can in the future be linked with other external libraries (which is why I don't really want to put it directly into all LDFLAGS of all modules), and
  • where the modules can be optionally included (that is, we could disable all built-in modules, but we would still need that simple library; similarly, we could enable all those built-in modules, but we only need one copy of that library).

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Aug 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Aug 1, 2025

Copy link
Copy Markdown
Contributor

Well if the archive isn't needed at all the linker will just leave it out. So you could put in LIBPYTHON directly. If you're concerned about it not existing at all you could check if it's completely absent and make an empty .a file in that case? It's a bit convoluted.

Or we could try wrapping the wasm linker and dropping the duplicated library.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Aug 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

If you're concerned about it not existing at all you could check if it's completely absent and make an empty .a file in that case? It's a bit convoluted.

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 static inline function. I'm still not ready for having a common interface for hashlib and hmac for querying available hash functions so we can live with header files only... Since I'm actually pretty busy for the next two weeks, it's better that I revert this and try to find a better alternative (at the same time, I think I'll be able to fix the issue we had with HACL* static libs where I just disabled the modules...)

Or we could try wrapping the wasm linker and dropping the duplicated library.

That was my idea, but we need to expand makefile variables before because the duplication appears because of LDFLAGS being added to MODLIBS. So there are ldflags coming from all cryptographic modules, and those contain .a. My latest attempt was to separate between the values passed to MODLIBS and those passed to the Makefile rule but I failed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

This comment was marked as resolved.

Sign in to view
@bedevere-bot

This comment was marked as resolved.

Sign in to view
@picnixz

picnixz commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

Hum... apparently my approach here works... I'll cleanup those hacks tomorrow.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Aug 2, 2025

Copy link
Copy Markdown
Member Author

Too hard to solve the conflicts, I'm going to start a fresh PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz picnixz closed this Aug 2, 2025
@picnixz
picnixz deleted the fix/hashlib/files-131876 branch October 11, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead Awaiting requested review from gpshead gpshead will be requested when the pull request is marked ready for review gpshead is a code owner
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland will be requested when the pull request is marked ready for review erlend-aasland is a code owner
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner will be requested when the pull request is marked ready for review AA-Turner is a code owner
@emmatyping emmatyping Awaiting requested review from emmatyping emmatyping will be requested when the pull request is marked ready for review emmatyping is a code owner
@itamaro itamaro Awaiting requested review from itamaro itamaro will be requested when the pull request is marked ready for review itamaro is a code owner
@corona10 corona10 Awaiting requested review from corona10 corona10 will be requested when the pull request is marked ready for review corona10 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@picnixz @bedevere-bot @zooba @hoodmane
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.