You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2025/03/10 16:06:23 UTC

[PR] feat: Improve shared linkage support [arrow-nanoarrow]

paleolimbot opened a new pull request, #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719

   This PR makes linking to the usual targets static linkage and adds a paired `_shared` target for users that want to opt in to shared nanoarrow linkage. This is intended to mirror what ADBC and Arrow do (with separated `_shared` and `_static` targets), except nanoarrow already had some existing targets that were intended to be used with static linkage anyway, so those target names were left as is.
   
   This PR also adds the appropriate import/export attribute for Windows.
   
   Closes #495, closes #718


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#discussion_r1992046218


##########
src/nanoarrow/nanoarrow.h:
##########
@@ -139,6 +139,20 @@
 
 #endif
 
+#if (defined _WIN32 || defined __CYGWIN__) && defined(NANOARROW_BUILD_DLL)
+#if defined(NANOARROW_EXPORT_DLL)
+#define NANOARROW_DLL __declspec(dllexport)
+#else
+#define NANOARROW_DLL __declspec(dllimport)
+#endif  // defined(NANOARROW_EXPORT_DLL)
+#elif !defined(NANOARROW_DLL)
+#if __GNUC__ >= 4
+#define NANOARROW_DLL __attribute__((visibility("default")))
+#else
+#define NANOARROW_DLL
+#endif  // __GNUC__ >= 4
+#endif

Review Comment:
   Yes distributing both .a and .so is certainly not uncommon. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2711122370

   If helpful for local debugging you might want to try setting this up through Meson, as that work is a lot simpler than what CMake requires. I think we used to even have this set up back in https://github.com/apache/arrow-nanoarrow/pull/531 but reverted due to some issues


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2711760000

   Love the individual commit names - really takes you along for the ride as a reader. Hope all is well!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2726194191

   > I have some thoughts about target naming and such but I need to take some more time and think about it and look some things up.
   
   Let me know here or elsewhere when you have time to circle back!
   
   > One thing is that I see a bunch of repeating stuff that should probably be refactored in a function, but that's true in general (see L436 onwards) so that could be done as a follow up.
   
   Great point! I've opened an issue to track this one: https://github.com/apache/arrow-nanoarrow/issues/726 .


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2740738923

   Thank you all for the reviews here!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2730199643

   > I think you should change nanoarrow explicitly to nanoarrow_static to be consistent 
   
   Done! I do like this better. I also added configurable test linkage for better feedback if this breaks for some reason.
   
   > then add a nanoarrow::nanoarrow alias to it 
   
   This is also sort of a breaking change (`target_link_libraries(... nanoarrow::nanoarrow)` vs `target_link_libraries(... nanoarrow)` and I think explicit `nanoarrow_static` is better (possibly at the expense of being slightly confusing to somebody who doesn't know what static linking is, like me threeish years ago 🙂 ). I'm personally OK with the breaking change here but could definitely be convinced otherwise.
   
   > For packaging the shared bit could be made a component that also changes which target gets the main alias.
   
   I'll leave this one for now since it doesn't affect the interface and because our compile times are very low but it's a great point that the average we-use-nanoarrow-as-a-subproject only needs one of the two.
   
   > nit: You should be able to create an INTERFACE target for the sources and use that instead of variables
   
   I'll add a note about this one in #726 since it's in the category of nice-to-have CMake improvements that I don't quite understand yet 🙂 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#discussion_r1991972772


##########
src/nanoarrow/nanoarrow.h:
##########
@@ -139,6 +139,20 @@
 
 #endif
 
+#if (defined _WIN32 || defined __CYGWIN__) && defined(NANOARROW_BUILD_DLL)
+#if defined(NANOARROW_EXPORT_DLL)
+#define NANOARROW_DLL __declspec(dllexport)
+#else
+#define NANOARROW_DLL __declspec(dllimport)
+#endif  // defined(NANOARROW_EXPORT_DLL)
+#elif !defined(NANOARROW_DLL)
+#if __GNUC__ >= 4
+#define NANOARROW_DLL __attribute__((visibility("default")))
+#else
+#define NANOARROW_DLL
+#endif  // __GNUC__ >= 4
+#endif

Review Comment:
   Thanks for taking a look!
   
   > To avoid keeping this up to date, I would recommend the usage of GenerateExportHeader
   
   I think the main reason I'd like to avoid this is that CMake is one of three supported ways of consuming nanoarrow (CMake, meson, make a single-file bundle and use your existing build system). 
   
   > is there a reason we want to install static and shared in parallel
   
   This seems to be what other libraries of our size/function are doing (e.g., zstd, ADBC, Arrow), and I think installing both in parallel works well for distributing via something like Homebrew (which often provides shared and static options to link against so that you can use it to distribute things like R packages and Python wheels). I think the main advantage comes when trying to link against a library from something that is not CMake but I'm not an expert here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2716406016

   @assignUser I hate to bug you again, but there's quite a big change to the CMake that would benefit from your 👀  if you have the bandwidth!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "m-kuhn (via GitHub)" <gi...@apache.org>.
m-kuhn commented on code in PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#discussion_r1992163407


##########
src/nanoarrow/nanoarrow.h:
##########
@@ -139,6 +139,20 @@
 
 #endif
 
+#if (defined _WIN32 || defined __CYGWIN__) && defined(NANOARROW_BUILD_DLL)
+#if defined(NANOARROW_EXPORT_DLL)
+#define NANOARROW_DLL __declspec(dllexport)
+#else
+#define NANOARROW_DLL __declspec(dllimport)
+#endif  // defined(NANOARROW_EXPORT_DLL)
+#elif !defined(NANOARROW_DLL)
+#if __GNUC__ >= 4
+#define NANOARROW_DLL __attribute__((visibility("default")))
+#else
+#define NANOARROW_DLL
+#endif  // __GNUC__ >= 4
+#endif

Review Comment:
   if cross-build-system support is a concern, I am with you the current approach is better 👍 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "m-kuhn (via GitHub)" <gi...@apache.org>.
m-kuhn commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2732109435

   Thanks for all the work!
   
   For my understanding, for downstream libraries that link to nanoarrow, this will mean to be used like this:
   
   ```
   target_link_libraries(main PRIVATE $<IF:$<TARGET_EXISTS:nanoarrow::nanoarrow_shared>,nanoarrow::nanoarrow_shared,nanoarrow::nanoarrow_static>)
   ```
   
   and there's no generic target that takes whatever has been built?
   
   ```
   target_link_libraries(main PRIVATE nanoarrow::nanoarrow)
   ```
   
   I'm asking as I never had the need to build static AND shared libraries side-by-side but had the need to build either one or the other depending on the target platform (e.g. shared for linux, static for ios) and for these cases the short version is nice in terms of readability.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#discussion_r1996558301


##########
.github/workflows/examples.yaml:
##########
@@ -107,15 +113,18 @@ jobs:
           ctest .
 
       - name: Comprehensive CMake example
+        shell: bash
+        env:
+          EXTRA_CMAKE_INSTALL: ${{ matrix.config.label == 'windows' && '--config=Debug' || '' }}
         run: |
           cd examples/cmake-scenarios
           ./build.sh
-          for dir in scratch/build*; do
-            ./${dir}/minimal_cpp_app;
-            echo ;
-          done
+          ./run.sh
 
       - name: Meson example
+        # This example doesn't seem to work on Windows
+        if: matrix.config.label != 'windows'

Review Comment:
   @WillAyd  I'll open an issue for this! I think it works but needs some environment-on-ci debugging!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2734806519

   > then add a nanoarrow::nanoarrow alias to it
   
   Ok, I added this + tests to make sure it works in both the fetch and installed cases. You and @m-kuhn are right that this is sort of annoying if you want the value of BUILD_SHARED_LIBS to do the work for you.
   
   > For my understanding, for downstream libraries that link to nanoarrow, this will mean to be used like this:
   
   It would probably be more like the `if(BUILD_SHARED_LIBS) ... target_link_libraries(... nanoarrow::nanoarrow_shared)` like Jacob demoed above...both targets are always available but you'd have to decide which one you want. There are some cases where you want to namespace the symbols + statically link even though you yourself are building a shared library. In any case, linking to `nanoarrow::nanoarrow` should do the same thing that it did before this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2727571748

   I think you should change `nanoarrow` explicitly to `nanoarrow_static` to be consistent and then add a `nanoarrow::nanoarrow` alias to it (I assume static will be the default? Or you could base that on `BUILD_SHARED_LIBS` like this
   ```cmake
   if(BUILD_SHARED_LIBS)
     add_library(nanoarrow::nanoarrow ALIAS nanoarrow_shared)
   else()
     add_library(nanoarrow::nanoarrow ALIAS nanoarrow_static)
   endif()
   ```
   This is mostly relevant when nanoarrow is built as a subproject. You could also add an option that allows toggling the building of either version (gflags does that). That way I don't have to compile everything twice if I only care about static.
   
   For packaging the shared bit could be made a component that also changes which target gets the main alias. (again something gflags does `find_package(gflags COMPONENTS shared)`
   
   nit: You should be able to create an INTERFACE target for the sources and use that instead of variables: https://cmake.org/cmake/help/latest/prop_tgt/INTERFACE_SOURCES.html
   
   I haven't used an interface target but it's a nice solution to avoid raw variables (which I am always happy about...)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "m-kuhn (via GitHub)" <gi...@apache.org>.
m-kuhn commented on code in PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#discussion_r1990689047


##########
src/nanoarrow/nanoarrow.h:
##########
@@ -139,6 +139,20 @@
 
 #endif
 
+#if (defined _WIN32 || defined __CYGWIN__) && defined(NANOARROW_BUILD_DLL)
+#if defined(NANOARROW_EXPORT_DLL)
+#define NANOARROW_DLL __declspec(dllexport)
+#else
+#define NANOARROW_DLL __declspec(dllimport)
+#endif  // defined(NANOARROW_EXPORT_DLL)
+#elif !defined(NANOARROW_DLL)
+#if __GNUC__ >= 4
+#define NANOARROW_DLL __attribute__((visibility("default")))
+#else
+#define NANOARROW_DLL
+#endif  // __GNUC__ >= 4
+#endif

Review Comment:
   To avoid keeping this up to date, I would recommend the usage of https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html which creates this code via cmake.
   
   ```
   set(CMAKE_CXX_VISIBILITY_PRESET hidden)
   set(CMAKE_VISIBILITY_INLINES_HIDDEN 1)
   generate_export_header(nanoarrow)
   
   install(FILES
    ${PROJECT_BINARY_DIR}/nanoarrow_export.h DESTINATION ${INCLUDE_INSTALL_DIR}
   )
   ```
   
   ```
   class NANOARROW_EXPORT SomeClass { ... }
   ```
   
   I do think that with this we get away with one exported target (no `nanoarrow_shared` target, or is there a reason we want to install static and shared in parallel?) and less boilerplate code to maintain



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#issuecomment-2716396551

   > Love the individual commit names - really takes you along for the ride as a reader. Hope all is well!
   
   I'm glad *you* enjoyed this 🙂  (all good...I really just needed to fire up my Windows machine for this one instead of trying to make CI do all the work)
   
   I didn't try too hard to get Meson to work here...I can follow up with that one some other time. Off the top of my head I think it's resolving the wrong `ninja` (maybe needs a .venv)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat: Improve shared linkage support [arrow-nanoarrow]

Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #719:
URL: https://github.com/apache/arrow-nanoarrow/pull/719#discussion_r1992078052


##########
CMakeLists.txt:
##########
@@ -416,6 +452,13 @@ foreach(target
     # Ensure NANOARROW_DEBUG is defined for debug builds
     target_compile_definitions(${target} PUBLIC "$<$<CONFIG:Debug>:NANOARROW_DEBUG>")
 
+    # Ensure NANOARROW_DLL is set when building and linking to the library
+    if(target MATCHES "_shared$")

Review Comment:
   Targets have a `TYPE` property that you could check here instead of the name: https://cmake.org/cmake/help/latest/prop_tgt/TYPE.html



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org