8000 dependency update by pca006132 · Pull Request #559 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

dependency update #559

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account rel 8000 ated emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Sep 14, 2023
Merged

dependency update #559

merged 7 commits into from
Sep 14, 2023

Conversation

pca006132
Copy link
Collaborator
@pca006132 pca006132 commented Sep 13, 2023 •
edited
Loading

Address #554 and #555.

  • Removed submodule for thrust and glm, use system library if possible and resort to git clone if there is no such package installed.
  • Allows users to not build js buildings when emscripten is used.
  • Updated documentation for our dependencies, and guide for building in offline mode without some of the packages.

@codecov
Copy link
codecov bot commented Sep 13, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (2055b9c) 91.13% compared to head (7550076) 91.13%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #559   +/-   ##
=======================================
  Coverage   91.13%   91.13%           
=======================================
  Files          35       35           
  Lines        4547     4547           
=======================================
  Hits         4144     4144           
  Misses        403      403

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pca006132
Copy link
Collaborator Author

hmmm, I have no idea why the nix builds fail.
will try to fix them later. I am unable to reproduce those problems on my machines even though flakes are supposed to be deterministic.

@elalish
Copy link
Owner
elalish commented Sep 13, 2023

Hmm, this sounds ominous:

CMake Warning at build/_deps/tbb-src/CMakeLists.txt:118 (message):
  You are building oneTBB as a static library.  This is highly discouraged
  and such configuration is not supported.  Consider building a dynamic
  library to avoid unforeseen issues.

@pca006132
Copy link
Collaborator Author

indeed, but that can easily be fixed anyway

@pca006132
Copy link
Collaborator Author

ok, the thrust cmake problem with nix is due to cmake non-deterministically applying the config files... I have no idea why this happens but I have worked around it by overwriting the output into a known correct result.
this only applies to our CI, and will not affect other packages.

pca006132
pca006132 commented Sep 13, 2023
View reviewed changes
.github/workflows/manifold.yml
@@ -219,7 +219,7 @@ jobs:
- uses: actions/checkout@v3
with:
submodules: recursive
- uses: cachix/install-nix-action@v15
- uses: cachix/install-nix-action@v22
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 later found that the nix non-deterministic issue is not related to this, but updating should not cause any problem.

pca006132
pca006132 commented Sep 13, 2023
View reviewed changes
test/CMakeLists.txt
add_library(GTest::GTest ALIAS gtest)
add_library(GTest::Main ALIAS gtest_main)
if(NOT TARGET GTest::gtest_main)
add_library(GTest::gtest_main ALIAS gtest_main)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GTest::Main may not exist, but GTest::gtest_main should exist for every version.

pca006132
pca006132 commented Sep 13, 2023
View reviewed changes
src/utilities/CMakeLists.txt
GIT_REPOSITORY https://github.com/oneapi-src/oneTBB.git
GIT_TAG v2021.10.0
GIT_SHALLOW TRUE
GIT_PROGRESS TRUE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

originally I wanted to use find_package for TBB as well (in addition to pkg-config), but it seems that the FindTBB.cmake provided in some versions are broken, and calling find_package(tbb) will cause cmake error, so I just keep the old behavior.

@pca006132
Copy link
Collaborator Author

It seems that BUILD_SHARED_LIB=ON behaves weirdly on Windows, so tbb will still be built statically.
Actually I found this to be quite weird, as tbb should be built as dynamic library by default and we did not set BUILD_SHARED_LIB in our library.

elalish
elalish approved these changes Sep 14, 2023
View reviewed changes
Copy link
Owner
@elalish elalish 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, and thanks for the docs updates too!

src/utilities/CMakeLists.txt
else()
target_include_directories(${PROJECT_NAME} INTERFACE
${THRUST_ROOT}
${THRUST_ROOT}/dependencies/libcudacxx/include
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a little surprised CMake doesn't take care of this automatically...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CMake should be able to handle it, but I found that for some reason FetchContent does not execute the thrust cmake file correctly. This is what we did previously as well when using submodules.

@pca006132
Copy link
Collaborator Author

I will merge this after I ensure that the python wheel build is working.

@pca006132
Copy link
Collaborator Author

Hmm... Nice job windows

CUSTOMBUILD : error : unable to create file libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_until_deadlock_bug.pass.cpp: Filename too long [C:\Users\runneradmin\AppData\Local\Temp\tmp6pcu43tk\build\_deps\thrust-subbuild\thrust-populate.vcxproj]

Never thought of filename too long will be an issue.

@pca006132 pca006132 merged commit 206871a into elalish:master Sep 14, 2023
@elalish elalish mentioned this pull request Nov 3, 2023
Merged
cartesian-theatrics pushed a commit to cartesian-theatrics/manifold that referenced this pull request Mar 11, 2024
@pca006132
dependency update (elalish#559)
a1df88f 
* dependency update

* revert BUILD_SHARED_LIBS changes

* test

* .

* don't try weird stuff with windows

* update

* update CI
@pca006132 pca006132 deleted the dependencies branch November 16, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elalish elalish elalish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pca006132 @elalish
0