Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 64
Add Emscripten libmpdec build step#615
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Goes together with: python/cpython#136997
zware commented Jul 22, 2025
Can these not be made already present on the host? |
hoodmane commented Jul 22, 2025
Do you mean that we should build them once and then install in some prefix directory which we reuse between jobs? |
hoodmane commented Jul 22, 2025
Building libmpdec is pretty fast, and it's nice for it to be reproducible. OTOH libssl takes several minutes to build... |
zware commented Jul 22, 2025
Right.
Windows builds use pre-built libssl binaries, partly for this reason. They also build some libraries (statically) on every build, so I'm OK with whichever direction you want to go for Emscripten :) |
freakboy3742 commented Jul 22, 2025
FWIW - this is the approach that iOS and Android use as well - we've got standalone repos (iOS, Android) to manage binary releases; the CPython CI builds then download/cache those releases. The complication for Emscripten is that we can't just add "bzip-1.0.8" and re-use it - we need to add "bzip-1.0.8, compiled for Emscripten 4.0.10". That's certainly possible, but it's definitely more complicated than the iOS/Android/macOS situation where the only real restriction is the minimum OS compatibility version. @hoodmane One thought - we could cache these builds as part of the whole "Emscripten version cache" thing we discussed at EuroPython. For the benefit of others - Emscripten doesn't provide any ABI compatibility guarantees between versions. We're effectively going to have to say "Python 3.14 uses Emscripten 4.0.11", and everything that is for Python3.14 will need to use that version of Emscripten. This means the buildbot will need to maintain multiple versions of Emscripten; the idea @hoodmane and I discussed at EuroPython was to add an We could then expand that to also include cached builds of libffi, mpdecimal, or whatever else is a dependency. We'd end up with a directory structure that looks something like: The idea would be that if you specify an Thoughts? |
hoodmane commented Jul 23, 2025
Makes sense to me. libmpdec only takes about 8 seconds to build though, so could we merge it first like this and then do the rearrangement with two libraries? I feel better about having two libs before making this change. Though I'm also okay with adding |
freakboy3742 commented Jul 24, 2025
I'd be fine with merging this as is, and then re-arranging as part of the larger refactor of the build script. |
freakboy3742 commented Jul 24, 2025
The complication is that the 3.14 branch is now locked until 3.14.0 final, so we can't merge this change to the buildbot without breaking 3.14 builds. |
StanFromIreland commented Jul 24, 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.
It may be possible for the step to be made optional depending on the branch? Similar things have been done before I think: buildmaster-config/master/custom/factories.py Lines 309 to 314 in 847d2ed
patchSubject: [PATCH] Make compile step not run on 3.14 --- Index: master/custom/factories.py IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 ===================================================================diff --git a/master/custom/factories.py b/master/custom/factories.py--- a/master/custom/factories.py (revision cacc9671eed1382c2b5cbdbde4e71a47a5c2164d)+++ b/master/custom/factories.py (date 1753342781896)@@ -1339,7 +1339,7 @@ buildersuffix = ".emscripten" factory_tags = ["emscripten"] - def setup(self, **kwargs):+ def setup(self, branch, **kwargs): compile_environ ={"PATH": os.pathsep.join([ "/home/emscripten/emsdk", @@ -1368,12 +1368,19 @@ name="Compile host libFFI", command=["python3", "Tools/wasm/emscripten", "make-libffi"], env=compile_environ, - ),- Compile(- name="Compile host libmpdec",- command=["python3", "Tools/wasm/emscripten", "make-mpdec"],- env=compile_environ,- ),+ )+ ])++ # This is done because ...+ if branch != '3.14':+ self.addStep(+ Compile(+ name="Compile host libmpdec",+ command=["python3", "Tools/wasm/emscripten", "make-mpdec"],+ env=compile_environ,+ ))++ self.addSteps([ Configure( name="Configure host Python", command=["python3", "Tools/wasm/emscripten", "configure-host"], |
hoodmane commented Jul 24, 2025
Thanks for the suggestion @StanFromIreland! I did that. |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
hoodmane commented Jul 28, 2025
@freakboy3742 does it look okay to merge now with the check that @StanFromIreland suggested? |
freakboy3742 commented Jul 29, 2025
The complication with this approach is that PR builds (i.e., everything fired by the So, this change would mean we wouldn't have the ability to run CI builds on PRs against the 3.14 branch. That seems like fairly significant collateral damage, especially in the 3.14 RC window. |
freakboy3742 commented Jul 29, 2025
The mpdec changes have been merged into 3.14, so we can roll back to the "non-gated version" and merge. |
ryanking13 commented Sep 5, 2025
My two cents about this is that, indeed, Emscripten does not support ABI compatibility between versions, but I think it is mainly related to shared libraries, not static ones. Static WASM libraries are standard WASM modules without any custom sections, so I think it would be quite safe to link the library built with old Emscripten versions to the runtime built with a newer Emscripten. Maybe we can experiment this in Pyodide first and see if it works, wdyt @hoodmane? |
zware left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC the condition is no longer necessary
| factory_tags= ["emscripten"] | ||
| defsetup(self, **kwargs): | ||
| defsetup(self, *, branch, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| defsetup(self, *, branch, **kwargs): | |
| defsetup(self, **kwargs): |
| ]) | ||
| ifbranch!='3.14': | ||
| # Can enable on 3.14 if/when python/cpython#137066 is merged | ||
| self.addStep( | ||
| Compile( | ||
| name="Compile host libmpdec", | ||
| command=["python3", "Tools/wasm/emscripten", "make-mpdec"], | ||
| env=compile_environ, | ||
| ) | ||
| ) | ||
| self.addSteps([ | ||
| Configure( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ]) | |
| ifbranch!='3.14': | |
| # Can enable on 3.14 if/when python/cpython#137066 is merged | |
| self.addStep( | |
| Compile( | |
| name="Compile host libmpdec", | |
| command=["python3", "Tools/wasm/emscripten", "make-mpdec"], | |
| env=compile_environ, | |
| ) | |
| ) | |
| self.addSteps([ | |
| Configure( | |
| Compile( | |
| name="Compile host libmpdec", | |
| command=["python3", "Tools/wasm/emscripten", "make-mpdec"], | |
| env=compile_environ, | |
| ), | |
| Configure( |
Goes together with:
python/cpython#136997