Skip to content

Conversation

@wesm
Copy link
Member

@wesm wesm commented Jan 17, 2020

If something like this is not done, then libarrow.a cannot be used without obtaining the exact libjemalloc_pic.a that we build privately.

Open questions I need help with

  • Does this break any packaging builds?
  • How does this affect the INSTALL CMake commands (and ArrowTargets.cmake, etc.)? Ideally we want people to be able to use Arrow as a dependency in other CMake projects and for things to Just Work
  • Test with ARROW_MIMALLOC=ON on Windows

@wesm wesm requested review from kou and xhochy January 17, 2020 17:22
@nealrichardson
Copy link
Member

Should we do this for all bundled dependencies in a static build? See also https://github.com/apache/arrow/blob/master/r/inst/build_arrow_static.sh#L93-L95

@wesm
Copy link
Member Author

wesm commented Jan 17, 2020

@nealrichardson it would indeed make linking for third parties much easier. One problem with doing this for other external dependencies is that they may be part of a larger static library toolchain.

As an example, consider an application that uses libzstd.a in more than one place. If we bundle libzstd.a in libarrow.a then this makes things more difficult for the third party developer.

On the other hand, there's nothing harmful about bundling libjemalloc_pic.a because these symbols are exclusive to Arrow

@wesm
Copy link
Member Author

wesm commented Jan 17, 2020

That being said it might not be a bad idea to have an optional build option to generate a libarrow_all_dependencies.a

@nealrichardson
Copy link
Member

Right, but if we're building it in the arrow build process (ARROW_DEPENDENCY_SOURCE=bundled), no one else is using the one we build. And in fact we can't use the static arrow library we build unless we fish those dependencies out of the _ep directories that cmake makes.

@github-actions
Copy link

@wesm
Copy link
Member Author

wesm commented Jan 17, 2020

@nealrichardson yes, I agree that when we use the BUNDLED approach for a dependency, we should splice that dependency into libarrow.a rather than install a useless static library

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Does this break any packaging builds?

RPM build will be broken because we install new files. We need to update install file list: https://github.com/apache/arrow/blob/master/dev/tasks/linux-packages/apache-arrow/yum/arrow.spec.in#L239

How does this affect the INSTALL CMake commands (and ArrowTargets.cmake, etc.)? Ideally we want people to be able to use Arrow as a dependency in other CMake projects and for things to Just Work

It affects nothing. arrow_static in ArrowTargets.cmake still doesn't work.
arrow_static has INTERFACE_LINK_LIBRARIES property and it includes jemalloc::jemalloc and mimalloc::mimalloc. But we don't provide jemalloc::jemalloc and minmalloc::minmalloc yet. We need to provide these targets to ArrowTargets.cmake to support arrow_static.

Copy link
Member

Choose a reason for hiding this comment

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

The following will be better because we can use both jemalloc and mimalloc at the same time.

set(ARROW_BUNDLE_STATIC_LIBS)

if(ARROW_JEMALLOC)
  list(APPEND ARROW_BUNDLE_STATIC_LIBS jemalloc::jemalloc)
endif()

if(ARROW_MIMALLOC)
  list(APPEND ARROW_BUNDLE_STATIC_LIBS mimalloc::mimalloc)
endif()

Copy link
Member

Choose a reason for hiding this comment

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

We want to lib.exe instead of libtool on Windows, right?
We will be able to use ${CMAKE_LINKER} on MSVC case: set(BUNDLE_TOOL ${CMAKE_LINKER})

Copy link
Member

Choose a reason for hiding this comment

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

How about _plain or _raw?

@wesm
Copy link
Member Author

wesm commented Jan 20, 2020

I'll take another turn on this tomorrow (Monday). I will see if I can assemble a list of all bundled libraries (not just jemalloc/mimalloc) to merge into libarrow.a.

@wesm
Copy link
Member Author

wesm commented Jan 22, 2020

We'll have to tackle this more completely after 0.16.0 rather than rush something through right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment saying what this does and why? This is a potential future head scratcher for whoever will debug this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I certainly will

@kszucs kszucs force-pushed the glue-jemalloc-into-libarrow branch from 2300884 to 1cb9745 Compare February 7, 2020 10:04
xhochy
xhochy previously approved these changes Feb 10, 2020
@wesm
Copy link
Member Author

wesm commented Feb 11, 2020

I'm going to clean this up as soon as I can and try to include any BUNDLED dependencies in libarrow.a

@pitrou pitrou force-pushed the glue-jemalloc-into-libarrow branch from 1cb9745 to b57ee51 Compare April 22, 2020 15:54
@tobim
Copy link
Contributor

tobim commented Apr 28, 2020

Bundling jemalloc into libarrow.a is probably the right course, but altering the generated archive in a way that CMake doesn't understand complicates the target export quite a bit.

Did you consider directly adding the object files from jemalloc to the list of source files of libarrow? The approach is described on Stackoverflow.

Another alternative is to install the custom jemalloc.a. Here is a patch that outlines that approach (This does not take care of pkgconfig yet).

@wesm
Copy link
Member Author

wesm commented Apr 28, 2020

@tobim you're welcome to propose an alternative solution and demonstrate how third party applications would be expected to statically link. Since we have install both CMake targets and pkg-config there's a number of things to test

@wesm
Copy link
Member Author

wesm commented Jun 1, 2020

Closing this PR for now. Hopefully someone can pick up this project

@wesm wesm closed this Jun 1, 2020
@wesm
Copy link
Member Author

wesm commented Jul 9, 2020

This work has been superseded by #7696

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants