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

[javascript] Find and use emscripten cmake toolchain in CMakeLists.txt (JS 2/n) #4105

Closed
wants to merge 2 commits into from

Conversation

AmesingFlank
Copy link
Collaborator

Related issue = #3781

Assuming that the path to the emscripten is stored in the EMSCRIPTEN_DIR environment variable, this PR finds the cmake toolchain files needed to build taichi with emscripten.

@vercel
Copy link

vercel bot commented Jan 25, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/taichi-api-docs-preview/taichi/AEsugiErJrkAFvTjfBvQ7udAeeK5
✅ Preview: Canceled

[Deployment for 576bb92 canceled]

@netlify
Copy link

netlify bot commented Jan 25, 2022

✔️ Deploy Preview for docsite-preview ready!

🔨 Explore the source changes: 576bb92

🔍 Inspect the deploy log: https://app.netlify.com/sites/docsite-preview/deploys/61f0b404a952140008e8147f

😎 Browse the preview: https://deploy-preview-4105--docsite-preview.netlify.app

CMakeLists.txt Outdated
@@ -4,6 +4,17 @@

cmake_minimum_required(VERSION 3.12)

if((DEFINED TI_EMSCRIPTENED) AND TI_EMSCRIPTENED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use cmake's like option command?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. I added the option in #4093 already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But since TaichiCore.cmake is included after the project(taichi) call, that option isn't declared here. I didn't wanna redeclare the option either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that's the case that option should be moved here probably. Since it's really a platform option not a taichi feature option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that makes sense. I'll move it here.

@@ -4,6 +4,17 @@

cmake_minimum_required(VERSION 3.12)

if((DEFINED TI_EMSCRIPTENED) AND TI_EMSCRIPTENED)
if(DEFINED ENV{EMSCRIPTEN_DIR})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe consider using CMake variables directly? (i.e. -DEMSCRIPTEN_DIR=...) instead of environment variables? Environment variables are quite easy to pollute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like that idea, but on the other hand we use environment variables everywhere else (TAICHI_CMAKE_ARGS, LLVM_DIR,... ) so I'm not sure. @ailzhang wdyt?

Copy link
Contributor

@ailzhang ailzhang Jan 29, 2022

Choose a reason for hiding this comment

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

@AmesingFlank sorry for the late reply! IMHO cmake variables are preferred way and we'll try to unify all cmake args into a singleTAICHI_CMAKE_ARGS env ;)

@feisuzhu
Copy link
Contributor

Closing ancient PR

@feisuzhu feisuzhu closed this Jan 16, 2023
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.

4 participants