Add support for non-vcpkg based builds #137

timkpaine · 2024-03-06T21:39:38Z

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)

conda/dev-environment-linux.yml

isuruf

Left one question. Otherwise looks good to me.

ngoldbaum · 2024-03-06T22:54:37Z

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

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 · 2024-03-07T15:55:09Z

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 · 2024-03-07T15:58:51Z

I defer to @robambalu.

timkpaine · 2024-03-07T16:02:26Z

@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 · 2024-03-07T16:05:43Z

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

timkpaine · 2024-03-07T16:06:33Z

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

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

ngoldbaum · 2024-03-07T16:11:31Z

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)

cpp/csp/adapters/utils/ProtobufHelper.h

ngoldbaum · 2024-03-11T15:37:17Z

@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 · 2024-03-11T16:48:27Z

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>

timkpaine added the part: build Issues and PRs related to the build process label Mar 6, 2024

timkpaine force-pushed the tkp/conda branch from f24f63f to dfed5ee Compare March 6, 2024 21:52

timkpaine requested review from ngoldbaum and isuruf March 6, 2024 22:11

isuruf reviewed Mar 6, 2024

View reviewed changes

conda/dev-environment-linux.yml Outdated Show resolved Hide resolved

isuruf reviewed Mar 6, 2024

View reviewed changes

conda/dev-environment-linux.yml Outdated Show resolved Hide resolved

isuruf previously approved these changes Mar 6, 2024

View reviewed changes

timkpaine dismissed isuruf’s stale review via 41da025 March 7, 2024 12:20

timkpaine force-pushed the tkp/conda branch from 1be0c2a to 41da025 Compare March 7, 2024 12:20

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

support non-vcpkg dependencies

28a69a4

support arrow 15

118f57f

Signed-off-by: Tim Paine <timothy.paine@cubistsystematic.com>

Move conda environment to folder, add CI checks for conda based

eed014c

workflow, enable setup.py to disable vcpkg builds Signed-off-by: Tim Paine <timothy.paine@cubistsystematic.com>

Rename wiki action and upgrade checkout therein, properly ignore docs…

a364c72

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

timkpaine force-pushed the tkp/conda branch from 41da025 to de3a51b Compare March 7, 2024 15:32

Fix python lint, remove unused flag

562f36b

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>

timkpaine force-pushed the tkp/conda branch from de3a51b to 562f36b Compare March 7, 2024 15:35

robambalu reviewed Mar 8, 2024

View reviewed changes

cpp/csp/adapters/utils/ProtobufHelper.h Outdated Show resolved Hide resolved

This was referenced Mar 11, 2024

Bump softprops/action-gh-release from 1 to 2 #139

Closed

Bump actions/checkout from 1 to 4 #140

Closed

address comments from PR, fixes #139

325a3c3

Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>

remove stuff that will break on conda-forge builds

fa489bb

Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>

robambalu self-requested a review March 13, 2024 12:55

robambalu approved these changes Mar 13, 2024

View reviewed changes

timkpaine merged commit d276ede into main Mar 13, 2024
16 checks passed

timkpaine deleted the tkp/conda branch March 13, 2024 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for non-vcpkg based builds #137

Add support for non-vcpkg based builds #137

timkpaine commented Mar 6, 2024

isuruf left a comment

ngoldbaum commented Mar 6, 2024

ngoldbaum commented Mar 7, 2024

ptomecek commented Mar 7, 2024

timkpaine commented Mar 7, 2024

ngoldbaum commented Mar 7, 2024

timkpaine commented Mar 7, 2024

ngoldbaum commented Mar 7, 2024 •

edited

Loading

ngoldbaum commented Mar 11, 2024

timkpaine commented Mar 11, 2024

Add support for non-vcpkg based builds #137

Add support for non-vcpkg based builds #137

Conversation

timkpaine commented Mar 6, 2024

isuruf left a comment

Choose a reason for hiding this comment

ngoldbaum commented Mar 6, 2024

ngoldbaum commented Mar 7, 2024

ptomecek commented Mar 7, 2024

timkpaine commented Mar 7, 2024

ngoldbaum commented Mar 7, 2024

timkpaine commented Mar 7, 2024

ngoldbaum commented Mar 7, 2024 • edited Loading

ngoldbaum commented Mar 11, 2024

timkpaine commented Mar 11, 2024

ngoldbaum commented Mar 7, 2024 •

edited

Loading