Skip to content

Use CMAKE_SYSROOT instead of EMSCRIPTEN_SYSROOT#27083

Open
xinitrcn1 wants to merge 1 commit into
emscripten-core:mainfrom
xinitrcn1:patch-1
Open

Use CMAKE_SYSROOT instead of EMSCRIPTEN_SYSROOT#27083
xinitrcn1 wants to merge 1 commit into
emscripten-core:mainfrom
xinitrcn1:patch-1

Conversation

@xinitrcn1

Copy link
Copy Markdown

No description provided.


# You **should** use CMAKE_SYSROOT, the one below is kept for
# compat for whichever soul used that variable...
set(CMAKE_SYSROOT "${_emcache_output}/sysroot")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the reason we don't do this is that emcc on it own already automatically includes the sysroot.

i.e. you never need to do emcc --sysroot=/path/to/emscripten/sysroot since emcc already knows exactly where its sysroot is. Its not something we expect user to pass.

I think we don't really want cmake to inject and additional --sysroot since it would be redundant.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we set the compiler to clang then it would make sense to pass things like --sysroot and --target=wasm32-unknown-emscripten, but when we use the emcc wrapper around clang they are not needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm aware emcc itself uses --sysroot..

HOWEVER
many projects use ${CMAKE_SYSROOT} for other miscellany purpouses, most notably:

  • pkg search
  • pkgconfig
  • find package
  • checks
  • unholy usages of "absolute paths" that depend on ${CMAKE_SYSROOT}

For example shadPS4 uses CMAKE_SYSROOT for checks and stuff, is it bad design? No! As per documentation CMAKE_SYSROOT MUST point to a valid sysroot, otherwise it's not a valid toolchain.

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