-
Notifications
You must be signed in to change notification settings - Fork 78
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
require pyjs >= 1.0.0
#588
Conversation
popd | ||
|
||
# Patch output | ||
python wasm_patches/patch_it.py |
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.
Is the patch not needed anymore?
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.
To be honest, I am not 100% sure. At least the pure pyjs
version I use in the pyjs-code-runner
does require this patch.
We can also leave it in.
What is the intent of uploading the files? are they currently consumed by any of our tools?
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.
If this is anyhow not used anywhere we can just remove the whole uploading part.
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.
If we still need the patch in https://github.com/emscripten-forge/recipes/blob/main/recipes/recipes_emscripten/xeus-python/build.sh#L25 then we should probably keep running it on the CI (just to make sure it actually runs).
What is the intent of uploading the files?
That was to be able to test them manually in jupyterlite-xeus-python prior to releasing, which was actually not possible without the relocate feature. But it may be possible now?
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.
Ok,then we leave it in, but I make an issue to check if we actually still need it
edit: here is the issue #589
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.
That was to be able to test them manually in jupyterlite-xeus-python prior to releasing, which was actually not possible without the relocate feature. But it may be possible now?
So the libpython
we get in the ci has the prefix $MAMBA_ROOT_PREFIX/envs/xeus-python-wasm-host
ie smth like /home/runner/micromamba/envs/xeus-python-wasm-host
. So the generated xeus wasm will have this prefix baked in.
So if you want to use these artifacts you need to create the env with micromamba
and relocated it to
/home/runner/micromamba/envs/xeus-python-wasm-host
This might work.
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.
@martinRenou btw I suspect if you just pass the relocate-prefix
argument empack pack env
it will just work. even if the env was not relocated! This can work because I explicitly set PYTHONHOME
to the relocated prefix in pyjs
https://github.com/emscripten-forge/pyjs/blob/main/include/pyjs/pre_js/init.js#L25-L26
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.
Thanks!
I also removed empack from the CI which was used to pack the env in a single js/data blob.
Since the single js/data blobs are outdated anyhow I removed that part