Skip to content

Conversation

@mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented Jan 10, 2025

Description

Please include a summary of changes, motivation and context for this PR.

This PR updates the build instructions so that we make use of CONDA_PREFIX. In the process of making this PR I noticed some bugs in the build (both ci and instructions) from not activating the wasm environment we create.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.72%. Comparing base (c805027) to head (72e53f7).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #209   +/-   ##
=======================================
  Coverage   80.72%   80.72%           
=======================================
  Files          19       19           
  Lines         970      970           
  Branches       93       93           
=======================================
  Hits          783      783           
  Misses        187      187           

@mcbarton mcbarton requested a review from anutosh491 January 10, 2025 20:59
@anutosh491
Copy link
Collaborator

Hmm, this might be something but I wont commit to it right now.

At the end, all xeus kernel should follow what jupyterlite-xeus has in its readme (https://github.com/jupyterlite/xeus?tab=readme-ov-file#build-the-kernel and https://github.com/jupyterlite/xeus?tab=readme-ov-file#build-the-jupyterlite-site)

We're following what is done by all other xeus kernels and won't like to change stuff unless done from the top or if something is obviously wrong.

Is there a particular advantage here ?

  1. I see we still need to export variables
  2. We rather need to activate envs twice in comparison to what we have.

Converting to draft for now

@anutosh491 anutosh491 marked this pull request as draft January 11, 2025 04:32
@mcbarton
Copy link
Collaborator Author

@anutosh491 At least for me on my Macbook the variable MAMBA_ROOT_PREFIX is not defined, and have to define manually. Activating the environment is better practice since it sets your environment variables correct. At the moment it only knows where the packages are because we essentially hardcode the path. You don't need the XEUS_CPP_INSTALL_DIR. Its just there until I do my PR which references a different location (which will reduce the size if our deployment and hopefully speed up the initial loading time)

@anutosh491
Copy link
Collaborator

anutosh491 commented Jan 11, 2025

At least for me on my Macbook the variable MAMBA_ROOT_PREFIX is not defined, and have to define manually

That shouldn't be the case. Maybe check your micromamba installation.

Activating the environment is better practice since it sets your environment variables correct

I mean cmake is expected to be outside this env and also as the readme for jupyterlite says it just wants a prefix location for the build, let's keep it that way.

Let's not deviate from what all kernels are doing (hence converted to draft, maybe once done from the top we can make the change)

@mcbarton
Copy link
Collaborator Author

mcbarton commented Jan 11, 2025

@anutosh491 I just realised we are already using conda_prefix when it comes to the non wasm build, so its not too far out there to use it in our wasm build too.

Also can you explain your comment about cmake being expected outside this environment. I don't see how activating the environment affects this 😅

Activating an environment doesn't stop an installed package outside that environment being accessible. My understanding is that activating the environment makes sure our system prioritises the ones in the environment being used. If you elaborate on your comment, then it should clear up my misunderstanding.

@anutosh491
Copy link
Collaborator

Also can you explain your comment about cmake being expected outside this environment. I don't see how activating the environment affects this 😅

Ahh nevermind, I might have missed something here. But my point being many jupyterlite maintainers would be interested in building xeus-cpp-lite (and xeus-r-lite and xeus-python-lite etc) in the coming days as quite some improvements are planned out on the jupyterlite-xeus side.

I don't plan on breaking consistency for one xeus kernel only. Maybe make a PR to jupyterlite-xeus directly changing the workflow there and then we can move this in.

@anutosh491
Copy link
Collaborator

Closing as done in jupyterlite/xeus#143
I myself have similar thoughts here.

@anutosh491 anutosh491 closed this Jan 14, 2025
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.

3 participants