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

require pyjs >= 1.0.0 #588

Merged
merged 6 commits into from
Apr 21, 2023
Merged

Conversation

DerThorsten
Copy link
Member

@DerThorsten DerThorsten commented Apr 21, 2023

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

@DerThorsten DerThorsten changed the title Req pyjs 1 0 0 require pyjs >= 1.0.0 Apr 21, 2023
@DerThorsten DerThorsten marked this pull request as ready for review April 21, 2023 08:09
popd

# Patch output
python wasm_patches/patch_it.py
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@martinRenou martinRenou merged commit 81b6c95 into jupyter-xeus:main Apr 21, 2023
7 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants