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

[ci] Use clang+Ninja to build Taichi on windows #3735

Merged
merged 13 commits into from
Dec 17, 2021
Merged

Conversation

bobcao3
Copy link
Collaborator

@bobcao3 bobcao3 commented Dec 7, 2021

Related issue = #

@netlify
Copy link

netlify bot commented Dec 7, 2021

✔️ Deploy Preview for jovial-fermat-aa59dc ready!

🔨 Explore the source changes: 525186b

🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/61bc00aae9cfd70007f0cb8e

😎 Browse the preview: https://deploy-preview-3735--jovial-fermat-aa59dc.netlify.app

@bobcao3
Copy link
Collaborator Author

bobcao3 commented Dec 7, 2021

/format

@bobcao3 bobcao3 force-pushed the bobcao3-patch-2 branch 2 times, most recently from 579728b to e474e20 Compare December 9, 2021 00:48
@bobcao3 bobcao3 marked this pull request as ready for review December 9, 2021 05:25
@bobcao3 bobcao3 changed the title [ci] Use clang to build windows (Test) [ci] Use clang to build windows Dec 9, 2021
@bobcao3
Copy link
Collaborator Author

bobcao3 commented Dec 9, 2021

Cache is all miss, not sure why. The build works

@bobcao3 bobcao3 requested review from frostming, Leonz5288, k-ye and ailzhang and removed request for Leonz5288 December 9, 2021 05:26
@bobcao3 bobcao3 changed the title [ci] Use clang to build windows [ci] Use clang+Ninja to build Taichi on windows Dec 9, 2021
@frostming
Copy link
Collaborator

Cache is all miss, not sure why. The build works

Maybe need to add -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache

@bobcao3
Copy link
Collaborator Author

bobcao3 commented Dec 9, 2021

Cache is all miss, not sure why. The build works

Maybe need to add -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache

No need. ccache is called, it's just that ccache thinks it's all miss. (in the cmakelist it detects the ccache program and automatically set the launchers)

setup.py Outdated
@@ -187,7 +187,7 @@ def prepare_package(self):
shutil.copy(moltenvk_path,
os.path.join(target, 'libMoltenVK.dylib'))
else:
shutil.copy('runtimes/Release/taichi_core.dll',
shutil.copy('runtimes/taichi_core.dll',
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC do we need Release here (seems like we explicitly opened a Release folder above for ninja as well) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The release folder is generated by msbuild. I can't think of a good way to detect whether msbuild or ninja is used. Maybe just enforce ninja?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this line already implicitly enforced ninja? ;) If so we'll need to be extra careful and make sure the dev install is updated correctly. ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes... kind of sad, but I can't think of a way to cleanly deal with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bobcao3 I don't mind switching to ninja for now ;) but this PR better stays consistent ;) . For example, if (CMAKE_GENERATOR EQUAL "Ninja") above kinda hints that we still support msbuild :D
If we are sure ninja is easy to install on windows and we know how to update dev install doc, we could just error out on msbuild in cmake?

@lin-hitonami
Copy link
Contributor

Do all the compilations happen in C:/Users/runneradmin/AppData/Local/Temp/pip-req-build-n_d7za0z/ ? This seems to be a randomly generated directory and it needs some configuration to enable sharing caches when compiling in different directories. See https://ccache.dev/manual/4.5.1.html#_compiling_in_different_directories . (btw sccache does not support that at all)

@frostming
Copy link
Collaborator

frostming commented Dec 9, 2021

Do all the compilations happen in C:/Users/runneradmin/AppData/Local/Temp/pip-req-build-n_d7za0z/ ? This seems to be a randomly generated directory and it needs some configuration to enable sharing caches when compiling in different directories. See https://ccache.dev/manual/4.5.1.html#_compiling_in_different_directories . (btw sccache does not support that at all)

Yes, because it is built by pip, which sets up a temp dir for build. Change it to python setup.py install to check if cache hits, but I have to mention invoking setup.py directly is not a good practice.

Update win_build.ps1
@ailzhang
Copy link
Contributor

@frostming

but I have to mention invoking setup.py directly is not a good practice.

Is it better to use pip install . instead ? If so feel free to send PRs to update our CI & doc sites :D!

Copy link
Contributor

@ailzhang ailzhang left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!!

if ($install) {
if ($develop) {
python -m pip install -v -e .
python setup.py develop
Copy link
Contributor

Choose a reason for hiding this comment

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

These commands are kinda equivalent (and IIUC pip install version is preferred :P so let's keep this unchanged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the pip version caching won't work because it uses a different path every time and the generators use absolute path everywhere

setup.py Outdated
@@ -187,7 +187,7 @@ def prepare_package(self):
shutil.copy(moltenvk_path,
os.path.join(target, 'libMoltenVK.dylib'))
else:
shutil.copy('runtimes/Release/taichi_core.dll',
shutil.copy('runtimes/taichi_core.dll',
Copy link
Contributor

Choose a reason for hiding this comment

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

@bobcao3 I don't mind switching to ninja for now ;) but this PR better stays consistent ;) . For example, if (CMAKE_GENERATOR EQUAL "Ninja") above kinda hints that we still support msbuild :D
If we are sure ninja is easy to install on windows and we know how to update dev install doc, we could just error out on msbuild in cmake?

@bobcao3
Copy link
Collaborator Author

bobcao3 commented Dec 13, 2021

I tried CMAKE_GENERATOR but it doesn't really work, it did not detect Ninja at all so... not sure

@frostming
Copy link
Collaborator

The changes made here LGTM and I hope this will be landed in the master soon as it dramatically accelerates the windows build 💯

We can figure out how to use the preferred build approach pip install later in another PR.

@ailzhang
Copy link
Contributor

@bobcao3 Would you mind resolving the merge conflict so that we can get it in? Thanks a lot!

```shell
.\build_taichi.ps1
```
```shell
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a three-space indentation to this code block


4. Build taichi by using `python setup.py develop`

:::note
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we just add this part (or move from other places)? I somehow couldn't find where it was moved from hmmm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously we asked the user to run build_taichi.ps1. Now we are asking them to build it through command line, the build script is for CI, not really for end user

@bobcao3
Copy link
Collaborator Author

bobcao3 commented Dec 17, 2021

/format

Copy link
Contributor

@ailzhang ailzhang left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

6 participants