Use CPM for pulling in units, remove conan from conda build path#3777
Merged
Use CPM for pulling in units, remove conan from conda build path#3777
Conversation
MridulS
force-pushed
the
cpm_llnl_units
branch
from
cebdaf9 to
c3685b0
Compare
Member
Author
|
All builds for conda and cibuildwheel passed in https://github.com/scipp/scipp/actions/runs/18698001628 and this is ready for another review. |
Member
SimonHeybrock
left a comment
SimonHeybrock
left a comment
There was a problem hiding this comment.
Looks good, small questions.
CMakeLists.txt
Outdated
| # conda don't seem to play well with this. | ||
| set(CMAKE_CXX_SCAN_FOR_MODULES 0) | ||
|
|
||
| if(APPLE AND NOT SKBUILD) |
Member
There was a problem hiding this comment.
Is SKBUILD active for all our builds, both PyPI and conda-forge?
Member
Author
There was a problem hiding this comment.
no, SKBUILD is only for pypi wheel building.
CMakeLists.txt
Outdated
Comment on lines
+16
to
+20
| set(CMAKE_THREAD_LIBS_INIT "-lpthread") | ||
| set(CMAKE_HAVE_THREADS_LIBRARY 1) | ||
| set(CMAKE_USE_WIN32_THREADS_INIT 0) | ||
| set(CMAKE_USE_PTHREADS_INIT 1) | ||
| set(THREADS_PREFER_PTHREAD_FLAG ON) |
Member
There was a problem hiding this comment.
So this is not disabling threading but using another kind?
Member
Author
There was a problem hiding this comment.
Yes, this is what works via clang compilers
Member
There was a problem hiding this comment.
What does it actually do? Does it change how TBB works? I don't think we use std::thread anywhere ourselves.
jl-wynen
reviewed
Oct 22, 2025
CMakeLists.txt
Outdated
Comment on lines
+16
to
+20
| set(CMAKE_THREAD_LIBS_INIT "-lpthread") | ||
| set(CMAKE_HAVE_THREADS_LIBRARY 1) | ||
| set(CMAKE_USE_WIN32_THREADS_INIT 0) | ||
| set(CMAKE_USE_PTHREADS_INIT 1) | ||
| set(THREADS_PREFER_PTHREAD_FLAG ON) |
Member
There was a problem hiding this comment.
What does it actually do? Does it change how TBB works? I don't think we use std::thread anywhere ourselves.
| include(scipp-conan) | ||
| find_package(LLNL-Units REQUIRED) | ||
| # cpm installs llnl units for conda and wheels both | ||
| include(scipp-cpm) |
Member
There was a problem hiding this comment.
Why do we only use scipp-cpm if we also use conan? Seems the wrong way around.
Member
Author
There was a problem hiding this comment.
I'm not sure I understand, this if clause is just skipping all network calls, conan and cpm are both not included as they both fetch remote sources.
Member
There was a problem hiding this comment.
Can you rename the variable? SKIP_CONAN sounds like it only controls conan, not network access.
| FILES "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config.cmake" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config-version.cmake" | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/cmake/${PROJECT_NAME}-conan.cmake" | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/cmake/${PROJECT_NAME}-cpm.cmake" |
Member
There was a problem hiding this comment.
Does this need to be conditional like include(scipp-cpm)?
Member
Author
There was a problem hiding this comment.
I guess we can remove from conan and cpm.cmake as if you are building for nix you don't need both of them, but I'm not even sure what will survive (cpm/conan/something else) by the time I am done with this 😓
Member
Author
|
I was trying to recreate the state where I was getting the threads not found error, but I can't recreate the issue :/ I can just remove the whole threading bit from this PR then :) The logs from one of the conda build failures are below: Details |
Member
Author
|
The builds for conda and pypi are available in the last commit https://github.com/scipp/scipp/actions/runs/18720040713?pr=3777 |
jl-wynen
approved these changes
Oct 23, 2025
SimonHeybrock
approved these changes
Oct 23, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.