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

Update Emscripten CI #416

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Conversation

anutosh491
Copy link
Contributor

@anutosh491 anutosh491 commented Jan 9, 2025

  1. Clones emsdk and uses it rather than fetching emsdk from conda-forge (just like xeus-python, xeus-cpp etc)

  2. I think CMAKE_FIND_ROOT_PATH_MODE_PACKAGE is being used wrongly.

Cmake docs say that there are only 3 values this variables accepts

i) ONLY
ii) NEVER
iii) BOTH

What we are using is something like CMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON which doesn't make sense.

As the docs say CMAKE_FIND_ROOT_PATH_MODE_PACKAGE basically controls two variables
i) CMAKE_FIND_ROOT_PATH
ii) CMAKE_SYSROOT

As far as my understanding goes we just need to control the CMAKE_FIND_ROOT_PATH that cmake uses while cross-compiling.

When cross-compiling (for example, when using Emscripten), CMake normally uses the host system's root directory as the search root. CMAKE_FIND_ROOT_PATH allows you to override this behaviour and specify alternate directories that CMake should treat as the root for package discovery.

So in this case we don't even need to control both vars through CMAKE_FIND_ROOT_PATH_MODE_PACKAGE as CMAKE_SYSROOT is not really of use to us.

@anutosh491
Copy link
Contributor Author

cc @martinRenou

@@ -3,6 +3,4 @@ channels:
- conda-forge
dependencies:
- cmake
- emsdk >=3.1.11
Copy link
Member

Choose a reason for hiding this comment

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

Yes 👍🏽 This one was a nasty hack that I don't want to maintain 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review.

Feel free to merge once you're happy ;)

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 a22275a into jupyter-xeus:main Jan 9, 2025
10 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.

2 participants