Skip to content

Conversation

@wesm
Copy link
Member

@wesm wesm commented Jul 9, 2020

This PR is a renewed attempt to address the brokenness of our static libraries and their exported CMake targets.

To summarize what's wrong: if any BUNDLED library source is used, or if the jemalloc or mimalloc allocators are used, then the libarrow.a static library that we've been installing is unusable. If someone tries to link with it, they get a linker error looking for the missing dependencies. Additionally, the CMake target names exported for arrow_static includes the list of all of these bundled libraries, so even attempting to use the arrow_static target does not work since the third party CMake build cannot find the missing dependencies.

As one downstream problem caused by this, we've been unable thus far to ship our non-system allocators in packages (R in particular) that do static linking.

Since it is neither safe nor reasonable to expect third party projects to attempt to provide substitute static libraries for the missing dependencies, I have implemented the following:

  • Using an MIT-licensed CMake solution, I splice together all of the bundled static libraries to create and install libarrow_bundled_dependencies.a (and arrow_bundled_dependencies.lib with MSVC). I have verified that this bundle is viable and can be used to create statically linked executables
  • Bundled dependencies are no longer exported in ArrowTargets-*.cmake. I spent a bunch of time trying to figure out how to get CMake to automatically pull in the "dependency bundle" when using arrow_static, but I couldn't figure out how to do it.
  • Added a ARROW_LINK_SHARED option to cpp/examples/minimal_build/CMakeLists.txt to show how to create statically linked executables using arrow_static and the dependency bundle.
  • Added a "run_static.sh" script to enable a bundled statically-linked build to be tested with Docker (perhaps we should add this to the CI job?)
  • Added "run_static.bat" to exhibit a statically linked build with MSVC on Windows

Finally, I verified that I can now successfully create statically linked builds now on Linux, macOS, and Windows.

@github-actions
Copy link

github-actions bot commented Jul 9, 2020

@wesm
Copy link
Member Author

wesm commented Jul 9, 2020

The CI failure https://github.com/apache/arrow/pull/7696/checks?check_run_id=855635184 is because ASF JIRA is hurting right now for some reason

@nealrichardson
Copy link
Member

@github-actions crossbow submit -g r

@github-actions
Copy link

github-actions bot commented Jul 9, 2020

Revision: 199af083138858c96f1c7cd8a063b165f3c46a5e

Submitted crossbow builds: ursa-labs/crossbow @ actions-395

Task Status
homebrew-r-autobrew TravisCI
test-conda-r-4.0 Github Actions
test-r-linux-as-cran Github Actions
test-r-rhub-ubuntu-gcc-release Azure
test-r-rocker-r-base-latest Azure
test-r-rstudio-r-base-3.6-bionic Azure
test-r-rstudio-r-base-3.6-centos6 Azure
test-r-rstudio-r-base-3.6-centos8 Azure
test-r-rstudio-r-base-3.6-opensuse15 Azure
test-r-rstudio-r-base-3.6-opensuse42 Azure
test-ubuntu-18.04-r-sanitizer Azure

@wesm
Copy link
Member Author

wesm commented Jul 9, 2020

@tobim FYI, this approach should be less offensive than what I was doing previously (hacking libarrow.a directly)

@wesm
Copy link
Member Author

wesm commented Jul 9, 2020

@nealrichardson the "test-conda-r-4.0" failure doesn't appear to be related to this patch

@wesm
Copy link
Member Author

wesm commented Jul 9, 2020

Homebrew seems to fail in the jemalloc EP, verbose thirdparty build could reveal more

[ 11%] Performing install step for 'jemalloc_ep'
cd /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/hbtmp/apache-arrow-20200709-78324-1gv9ydo/build/jemalloc_ep-prefix/src/jemalloc_ep && /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/build-apache-arrow/Cellar/cmake/3.12.2/bin/cmake -P /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/hbtmp/apache-arrow-20200709-78324-1gv9ydo/build/jemalloc_ep-prefix/src/jemalloc_ep-stamp/jemalloc_ep-install-RELEASE.cmake
CMake Error at /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/hbtmp/apache-arrow-20200709-78324-1gv9ydo/build/jemalloc_ep-prefix/src/jemalloc_ep-stamp/jemalloc_ep-install-RELEASE.cmake:16 (message):
  Command failed: 2
   '/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/build-apache-arrow/Library/Homebrew/shims/mac/super/make' 'install'
  See also
    /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/hbtmp/apache-arrow-20200709-78324-1gv9ydo/build/jemalloc_ep-prefix/src/jemalloc_ep-stamp/jemalloc_ep-install-*.log
make[2]: *** [jemalloc_ep-prefix/src/jemalloc_ep-stamp/jemalloc_ep-install] Error 1
make[1]: *** [CMakeFiles/jemalloc_ep.dir/all] Error 2
make: *** [all] Error 2
Do not report this issue to Homebrew/brew or Homebrew/core!

@nealrichardson
Copy link
Member

Linux seems to be working smoothly; I'll poke at macOS autobrew. On mingw, looks like there's an issue compiling mimalloc: https://github.com/apache/arrow/pull/7696/checks?check_run_id=855852766#step:7:2254

@nealrichardson
Copy link
Member

@github-actions crossbow submit homebrew-r-autobrew

@github-actions
Copy link

github-actions bot commented Jul 9, 2020

Revision: 706e50be600ee8d8ee33cffdb9611720e1dcd05c

Submitted crossbow builds: ursa-labs/crossbow @ actions-396

Task Status
homebrew-r-autobrew TravisCI

@wesm wesm force-pushed the dependency-bundling branch from 706e50b to aeb3830 Compare July 9, 2020 23:28
@wesm
Copy link
Member Author

wesm commented Jul 9, 2020

The mimalloc issue seems to only affect the DLL, which we don't need, so I disabled it

@nealrichardson
Copy link
Member

@github-actions crossbow submit homebrew-r-autobrew

@github-actions
Copy link

github-actions bot commented Jul 9, 2020

Revision: aeb3830e5935904a8a719a4aaf6c8131d2dbc141

Submitted crossbow builds: ursa-labs/crossbow @ actions-397

Task Status
homebrew-r-autobrew TravisCI

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

@nealrichardson try adding -lbcrypt -lpsapi to the linker setup, it seems like those might be required by mimalloc on Windows/MinGW

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

Confirming the new transitive dependencies from mimalloc:

if(WIN32)
  list(APPEND mi_libraries psapi shell32 user32 bcrypt)
else()

https://github.com/microsoft/mimalloc/blob/master/CMakeLists.txt#L163

@wesm wesm force-pushed the dependency-bundling branch from aeb3830 to da68e09 Compare July 10, 2020 00:45
@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

I'm having a hard time reading the Homebrew job output -- was the prior failure a flake?

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

@github-actions crossbow submit homebrew-r-autobrew

@github-actions
Copy link

Revision: da68e097ad07c0b9036e8175349bad2ba30787be

Submitted crossbow builds: ursa-labs/crossbow @ actions-398

Task Status
homebrew-r-autobrew TravisCI

@nealrichardson
Copy link
Member

IDK why the first autobrew job failed but the last (third) succeeded. Don't think that turning on verbosity should have fixed it. I think the second one failed because you force-pushed before the crossbow job started.

@nealrichardson
Copy link
Member

Windows Rtools builds are green now and autobrew passed again, so should we revert 12c7803, confirm autobrew is still passing, and merge this?

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

Yes that sounds good. @kou do you have any concerns about this? Let’s wait a bit for more comments

@kou
Copy link
Member

kou commented Jul 10, 2020

@github-actions crossbow submit -g linux

@github-actions
Copy link

Revision: da68e097ad07c0b9036e8175349bad2ba30787be

Submitted crossbow builds: ursa-labs/crossbow @ actions-399

Task Status
centos-6-amd64 Github Actions
centos-7-aarch64 TravisCI
centos-7-amd64 Github Actions
centos-8-aarch64 TravisCI
centos-8-amd64 Github Actions
debian-buster-amd64 Github Actions
debian-buster-arm64 TravisCI
debian-stretch-amd64 Github Actions
debian-stretch-arm64 TravisCI
ubuntu-bionic-amd64 Github Actions
ubuntu-bionic-arm64 TravisCI
ubuntu-eoan-amd64 Github Actions
ubuntu-eoan-arm64 TravisCI
ubuntu-focal-amd64 Github Actions
ubuntu-focal-arm64 TravisCI
ubuntu-xenial-amd64 Github Actions
ubuntu-xenial-arm64 TravisCI

@tobim
Copy link
Contributor

tobim commented Jul 10, 2020

Can you apply this patch to synchronize to mimalloc?

diff --git a/cmake_modules/ThirdpartyToolchain.cmake b/cmake_modules/ThirdpartyToolchain.cmake
--- a/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cmake_modules/ThirdpartyToolchain.cmake
@@ -1317,6 +1317,7 @@ if(ARROW_JEMALLOC)
               "--with-jemalloc-prefix=je_arrow_"
               "--with-private-namespace=je_arrow_private_"
               "--without-export"
+              "--disable-shared"
               # Don't override operator new()
               "--disable-cxx" "--disable-libdl"
               # See https://github.com/jemalloc/jemalloc/issues/1237

@wesm wesm force-pushed the dependency-bundling branch from da68e09 to 1f24b9e Compare July 10, 2020 19:43
@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

@github-actions crossbow submit -g linux

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

I tried fixing the Linux packages, let's see if this works

@github-actions
Copy link

Revision: 1f24b9e

Submitted crossbow builds: ursa-labs/crossbow @ actions-400

Task Status
centos-6-amd64 Github Actions
centos-7-aarch64 TravisCI
centos-7-amd64 Github Actions
centos-8-aarch64 TravisCI
centos-8-amd64 Github Actions
debian-buster-amd64 Github Actions
debian-buster-arm64 TravisCI
debian-stretch-amd64 Github Actions
debian-stretch-arm64 TravisCI
ubuntu-bionic-amd64 Github Actions
ubuntu-bionic-arm64 TravisCI
ubuntu-eoan-amd64 Github Actions
ubuntu-eoan-arm64 TravisCI
ubuntu-focal-amd64 Github Actions
ubuntu-focal-arm64 TravisCI
ubuntu-xenial-amd64 Github Actions
ubuntu-xenial-arm64 TravisCI

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

The one test failure is spurious

pyarrow/tests/test_fs.py:1285: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pyarrow/_fs.pyx:439: in pyarrow._fs.FileSystem.get_file_info
    infos = GetResultValue(self.fs.GetFileInfo(selector))
pyarrow/error.pxi:122: in pyarrow.lib.pyarrow_internal_check_status
    return check_status(status)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
>   raise IOError(message)
E   OSError: When listing objects under key '' in bucket 'ursa-labs-taxi-data': AWS Error [code 99]: Unable to connect to endpoint
E   In ../src/arrow/filesystem/s3fs.cc, line 1098, code: ListObjectsV2(bucket, key, std::move(handle_results), std::move(handle_error))
E   In ../src/arrow/filesystem/s3fs.cc, line 1358, code: impl_->Walk(select, base_path.bucket, base_path.key, &results)

I verified that the minimal build still works on Linux (presumably it's therefore fine on macOS) and Windows/MSVC

@tobim
Copy link
Contributor

tobim commented Jul 10, 2020

We can't put the Arrow toolchain libraries in INTERFACE_LINK_LIBRARIES because they will be unrecognized by the third party project.

I still think that "-DARROW_DEPENDENCY_SOURCE=SYSTEM" should work with arrow-static.

For this case my opinion is that non-vendored dependencies should not be bundled. Normally I want use the SYSTEM libraries for dependencies. As a developer of a third-party I do not want arrow to add symbols it does not own to my static linking process.

May I suggest a different solution for that aspect of the problem: CMake provides a module for declaring dependencies in the templated _Project_Config.cmake. That allows you to insert your own dependencies to the target graph of the downstream project.

@nealrichardson
Copy link
Member

Re: system dependencies and static build, is https://issues.apache.org/jira/browse/ARROW-6312 related? I encountered this issue in https://issues.apache.org/jira/browse/ARROW-9303, and isn't this is a slightly different problem? Because your system dependencies could be shared libraries that you couldn't glue together anyway, right?

@tobim
Copy link
Contributor

tobim commented Jul 10, 2020

Re: system dependencies and static build, is https://issues.apache.org/jira/browse/ARROW-6312 related? I encountered this issue in https://issues.apache.org/jira/browse/ARROW-9303, and isn't this is a slightly different problem?

ARROW-6312 is the pkg-config variant of the approach I suggested for CMake.

Because your system dependencies could be shared libraries that you couldn't glue together anyway, right?

Luckily I have control over linking flavor of the libraries on my system search path.

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

I still think that "-DARROW_DEPENDENCY_SOURCE=SYSTEM" should work with arrow-static.
For this case my opinion is that non-vendored dependencies should not be bundled. Normally I want use the SYSTEM libraries for dependencies. As a developer of a third-party I do not want arrow to add symbols it does not own to my static linking process.

@tobim our build system does not select static libraries when using -DARROW_DEPENDENCY_SOURCE=SYSTEM so they would not be bundled in libarrow_bundled_dependencies.a. That's kind of the whole point of this PR -- only add static libraries to libarrow_bundled_dependencies.a that the Arrow build system built itself.

If arrow_static has shared library dependencies on system libraries, then some other approach will be required to inform CMake automatically when doing find_package(Arrow). It's definitely out of the scope for this PR.

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

@github-actions crossbow submit -g linux

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

Fingers crossed that I've fixed the gRPC bundling problem and the Linux packages work this time

@github-actions
Copy link

Revision: f43ce46

Submitted crossbow builds: ursa-labs/crossbow @ actions-404

Task Status
centos-6-amd64 Github Actions
centos-7-aarch64 TravisCI
centos-7-amd64 Github Actions
centos-8-aarch64 TravisCI
centos-8-amd64 Github Actions
debian-buster-amd64 Github Actions
debian-buster-arm64 TravisCI
debian-stretch-amd64 Github Actions
debian-stretch-arm64 TravisCI
ubuntu-bionic-amd64 Github Actions
ubuntu-bionic-arm64 TravisCI
ubuntu-eoan-amd64 Github Actions
ubuntu-eoan-arm64 TravisCI
ubuntu-focal-amd64 Github Actions
ubuntu-focal-arm64 TravisCI
ubuntu-xenial-amd64 Github Actions
ubuntu-xenial-arm64 TravisCI

@tobim
Copy link
Contributor

tobim commented Jul 10, 2020

our build system does not select static libraries when using -DARROW_DEPENDENCY_SOURCE=SYSTEM

This is not true.

only add static libraries to libarrow_bundled_dependencies.a that the Arrow build system built itself.

That's how it should be then. But by removing ARROW_STATIC_INSTALL_INTERFACE_LIBS you are breaking the case of external static dependencies. If you would only add those libs that are not BUNDLED to the list I'd be happy.

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

This is not true.

Sorry, my presumption was that when a system library is installed there is a shared library available. If there is both a shared and static library available for a dependency, the build system is supposed to select the shared one. But you are right if there is only a static library (e.g. if someone created an all-static "system" toolchain), then indeed static libraries would be selected. But they would not be added to libarrow_bundled_dependencies.a, and they aren't.

That's how it should be then. But by removing ARROW_STATIC_INSTALL_INTERFACE_LIBS you are breaking the case of external static dependencies. If you would only add those libs that are not BUNDLED to the list I'd be happy.

Well, static linking was not possible at all prior to this PR when using the recommended memory allocator (jemalloc). So nothing has been "broken" because it never worked before. The case of external static dependencies isn't tested at all -- in no CI or integration task. If this use case is important to you, we could use your help writing Dockerized integration tests that exhibit this scenario so that we aren't flying blind like we are now.

I agree with adding non-bundled static libraries to ARROW_STATIC_INSTALL_INTERFACE_LIBS but we have to write tests to demonstrate that it works and that it does not break the use cases represented in this PR.

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

+1. Let us address the ARROW_STATIC_INSTALL_INTERFACE_LIBS issue and associated testing as a follow up PR.

@wesm wesm closed this in afe5515 Jul 10, 2020
@wesm wesm deleted the dependency-bundling branch July 10, 2020 22:00
@tobim
Copy link
Contributor

tobim commented Jul 10, 2020

I'm fine with that. I'll try to find some time for it tomorrow.

@wesm
Copy link
Member Author

wesm commented Jul 10, 2020

Thanks. I just opened https://issues.apache.org/jira/browse/ARROW-9412

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.

4 participants