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

Fix faulty CMAKE_FIND_ROOT_PATH_MODE_PACKAGE used for the wasm build #204

Merged

Conversation

anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Jan 9, 2025

Description

I'm not sure which PR introduced CMAKE_FIND_ROOT_PATH_MODE_PACKAGE but it is being used wrongly.

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

  1. ONLY
  2. NEVER
  3. BOTH

What we are using is something like CMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON which doesn't make sense (its wrong but somehow the build just works)

Please tick all options which are relevant.

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

@anutosh491
Copy link
Collaborator Author

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 behavior 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 anutosh491 force-pushed the CMAKE_FIND_ROOT_PATH_MODE_PACKAGE branch from f2cfb8b to 5afd152 Compare January 9, 2025 08:15
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.72%. Comparing base (ac45e1f) to head (41ba265).

Additional details and impacted files

Impacted file tree graph

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

@anutosh491
Copy link
Collaborator Author

cc @mcbarton for reviews

It was a faulty thing on xeus (xeus-cookiecutter) which was responsible for this. I fixed it on xeus yersterday (jupyter-xeus/xeus#416)

We need to do the same on libraries that might affect xeus-cpp (I don't think other libraries have this mistake but cppinterop does use CMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON which needs to be fixed)

@mcbarton
Copy link
Collaborator

@anutosh491 If removing CMAKE_PREFIX_PATH from the cmake config shouldn't you also remove where you export this variable just before building?

@anutosh491 anutosh491 force-pushed the CMAKE_FIND_ROOT_PATH_MODE_PACKAGE branch from d991489 to 41ba265 Compare January 10, 2025 12:41
@anutosh491
Copy link
Collaborator Author

you also remove where you export this variable just before building?

Absolutely, that's not required for the build.

@anutosh491 anutosh491 requested a review from mcbarton January 10, 2025 12:53
@mcbarton mcbarton merged commit 412c51c into compiler-research:main Jan 10, 2025
12 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.

3 participants