Skip to content

Conversation

@timkpaine
Copy link
Member

This PR does a few things:

  • Add support for non-vcpkg based builds, right now only supporting a conda-based workflow
  • Add CI/CD to test conda-based build
  • Upgrade arrow C++ dependency to 15 to better work with arrow shared library (e.g. in conda)
  • Move conda dependency file to folder in preparation for more yaml files (e.g. Add a windows development environment specification #123), update docs to reflect this and switch to slightly easier micromamba-based workflow
  • Small fix for wiki action to ignore changes to docs (no need to waste CI resources for docs-only changes)

@timkpaine timkpaine added the part: build Issues and PRs related to the build process label Mar 6, 2024
@timkpaine timkpaine requested review from isuruf and ngoldbaum March 6, 2024 22:11
isuruf
isuruf previously approved these changes Mar 6, 2024
Copy link
Contributor

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Left one question. Otherwise looks good to me.

@ngoldbaum
Copy link
Contributor

Awesome, the conda build only takes a few minutes. The changes to the docs and github actions setup look fine to me.

@timkpaine timkpaine marked this pull request as ready for review March 7, 2024 12:21
isuruf and others added 4 commits March 7, 2024 10:31
Signed-off-by: Tim Paine <timothy.paine@cubistsystematic.com>
workflow, enable setup.py to disable vcpkg builds

Signed-off-by: Tim Paine <timothy.paine@cubistsystematic.com>
… changes in main build ci

Signed-off-by: Tim Paine <timothy.paine@cubistsystematic.com>
use micromamba shell for conda workflow
Use correct build command
add deps for slack and symphony adapters to conda env
cmake simplifications
bump arrow to 15 in conda env

Signed-off-by: Tim Paine <timothy.paine@cubistsystematic.com>
@ngoldbaum
Copy link
Contributor

If I merge with #83 and make the following hacky changes to the build on MacOS:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7581de9..179e8f2 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -132,18 +132,18 @@ if(MACOS)
     set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -undefined dynamic_lookup")
 
     # Support cross build
-    check_c_compiler_flag("-arch x86_64" x86_64Supported)
-    check_c_compiler_flag("-arch arm64" arm64Supported)
-
-    if(x86_64Supported AND arm64Supported)
-        set(CMAKE_OSX_ARCHITECTURES "x86_64;arm64" CACHE STRING "Build universal architecture for OSX" FORCE)
-    elseif(x86_64Supported)
-        set(CMAKE_REQUIRED_LINK_OPTIONS "-arch;x86_64")
-        set(CMAKE_OSX_ARCHITECTURES "x86_64" CACHE STRING "Build universal architecture for OSX" FORCE)
-    elseif(arm64Supported)
-        set(CMAKE_REQUIRED_LINK_OPTIONS "-arch;arm64")
-        set(CMAKE_OSX_ARCHITECTURES "arm64" CACHE STRING "Build universal architecture for OSX" FORCE)
-    endif()
+    #check_c_compiler_flag("-arch x86_64" x86_64Supported)
+    #check_c_compiler_flag("-arch arm64" arm64Supported)
+
+    #if(x86_64Supported AND arm64Supported)
+    #    set(CMAKE_OSX_ARCHITECTURES "x86_64;arm64" CACHE STRING "Build universal architecture for OSX" FORCE)
+    #elseif(x86_64Supported)
+    #    set(CMAKE_REQUIRED_LINK_OPTIONS "-arch;x86_64")
+    #    set(CMAKE_OSX_ARCHITECTURES "x86_64" CACHE STRING "Build universal architecture for OSX" FORCE)
+    #elseif(arm64Supported)
+    set(CMAKE_REQUIRED_LINK_OPTIONS "-arch;arm64")
+    set(CMAKE_OSX_ARCHITECTURES "arm64" CACHE STRING "Build universal architecture for OSX" FORCE)
+    #endif()
 endif()
 
 
diff --git a/setup.py b/setup.py
index ead52be..7ae1e0d 100644
--- a/setup.py
+++ b/setup.py
@@ -68,8 +68,8 @@ if platform.system() == "Darwin":
     os.environ["OSX_DEPLOYMENT_TARGET"] = os.environ.get("OSX_DEPLOYMENT_TARGET", "10.13")
     os.environ["MACOSX_DEPLOYMENT_TARGET"] = os.environ.get("OSX_DEPLOYMENT_TARGET", "10.13")
 
-if hasattr(platform, "mac_ver") and platform.mac_ver()[0].startswith("14"):
-    cmake_args.append("-DCSP_USE_LD_CLASSIC_MAC=ON")
+#if hasattr(platform, "mac_ver") and platform.mac_ver()[0].startswith("14"):
+#    cmake_args.append("-DCSP_USE_LD_CLASSIC_MAC=ON")
 
 if which("ccache"):
     cmake_args.append("-DCSP_USE_CCACHE=On")

Then I can build csp in a mac conda environment using dev-environment-linux.yml (although I did have to additionally install boost due to the use of boost vector in the clang PR). This build passes all the tests.

This seems like a clear win and I think we should merge it ASAP. @robambalu @ptomecek are you OK with this?

@ptomecek
Copy link
Collaborator

ptomecek commented Mar 7, 2024

I defer to @robambalu.

@timkpaine
Copy link
Member Author

@ngoldbaum your top set of cmake changes are not correct, they break x86 and unversal2 Mac builds and shouldn't be necessary for conda forge.

@ngoldbaum
Copy link
Contributor

I know, that's why I said they were hacky changes.

@timkpaine
Copy link
Member Author

I know, that's why I said they were hacky changes.

haha fair enough. what error does it do without them?

@ngoldbaum
Copy link
Contributor

ngoldbaum commented Mar 7, 2024

The first set of changes avoid this linker error:

[2/10] Linking CXX shared library lib/_cspimpl.so
FAILED: lib/_cspimpl.so
: && /Users/goldbaum/miniforge3/envs/csp/bin/arm64-apple-darwin20.0.0-clang++ -ftree-vectorize -fPIC -fstack-protector-strong -pipe -stdlib=libc++ -fvisibility-inlines-hidden -fmessage-length=0 -isystem /Users/goldbaum/miniforge3/envs/csp/include -std=c++17         -O3         -g0         -Wall         -Wno-deprecated-declarations         -Wno-deprecated          -O3 -DNDEBUG -arch x86_64 -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -mmacosx-version-min=14.0 -dynamiclib -Wl,-headerpad_max_install_names -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-rpath,/Users/goldbaum/miniforge3/envs/csp/lib -L/Users/goldbaum/miniforge3/envs/csp/lib -undefined dynamic_lookup -o lib/_cspimpl.so -install_name @rpath/_cspimpl.so cpp/csp/python/CMakeFiles/cspimpl.dir/cspimpl.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/Conversions.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/NumpyConversions.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyAdapterManager.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyAdapterManagerWrapper.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyConstAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyCppNode.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyEngine.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyGraphOutputAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyInputAdapterWrapper.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyBasketInputProxy.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyBasketOutputProxy.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyDynamicNode.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyFeedbackAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyInputProxy.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyNode.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyNodeWrapper.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyNumbaNode.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyNumpyAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyOutputAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyOutputAdapterWrapper.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyOutputProxy.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyPullInputAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyPushInputAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyPushPullInputAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyManagedSimInputAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyTimerAdapter.cpp.o cpp/csp/python/CMakeFiles/cspimpl.dir/PyConstants.cpp.o  -Wl,-rpath,@loader_path/  lib/_csptypesimpl.so  lib/libcsp_core_static.a  lib/libcsp_engine_static.a  lib/libcsp_core_static.a  lib/libcsp_types_static.a && :
ld: can't re-map file, errno=22 file 'lib/_csptypesimpl.so' for architecture x86_64

And the second set of changes avoid this compiler error:

[37/138] Linking CXX shared library lib/_csptypesimpl.so
FAILED: lib/_csptypesimpl.so
: && /Users/goldbaum/miniforge3/envs/csp/bin/arm64-apple-darwin20.0.0-clang++ -ftree-vectorize -fPIC -fstack-protector-strong -pipe -stdlib=libc++ -fvisibility-inlines-hidden -fmessage-length=0 -isystem /Users/goldbaum/miniforge3/envs/csp/include -std=c++17         -O3         -g0         -Wall         -Wno-deprecated-declarations         -Wno-deprecated          -O3 -DNDEBUG -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -mmacosx-version-min=14.0 -dynamiclib -Wl,-headerpad_max_install_names -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-rpath,/Users/goldbaum/miniforge3/envs/csp/lib -L/Users/goldbaum/miniforge3/envs/csp/lib -Wl,-ld_classic -undefined dynamic_lookup -o lib/_csptypesimpl.so -install_name @rpath/_csptypesimpl.so cpp/csp/python/CMakeFiles/csptypesimpl.dir/csptypesimpl.cpp.o cpp/csp/python/CMakeFiles/csptypesimpl.dir/CspTypeFactory.cpp.o cpp/csp/python/CMakeFiles/csptypesimpl.dir/PyCspEnum.cpp.o cpp/csp/python/CMakeFiles/csptypesimpl.dir/PyCspType.cpp.o cpp/csp/python/CMakeFiles/csptypesimpl.dir/PyStruct.cpp.o  -Wl,-rpath,@loader_path/  lib/libcsp_core_static.a  lib/libcsp_types_static.a && :
ld: library not found for -ld_classic
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

@ngoldbaum
Copy link
Contributor

@timkpaine were you planning to do any more on this before merging? I think if we merge this and tag a release I could do linux conda-forge packaging right away.

@timkpaine
Copy link
Member Author

Yes I have a few last things. I had to patch my fork to get it building on conda forge, I will resolve the one outstanding comment, bump the other action that needs upgrading, and fix the patch issue and then it should be good.

Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
@robambalu robambalu self-requested a review March 13, 2024 12:55
@timkpaine timkpaine merged commit d276ede into main Mar 13, 2024
@timkpaine timkpaine deleted the tkp/conda branch March 13, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part: build Issues and PRs related to the build process

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants