Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arrow-cpp: fix build by using internal gtest #52928

Closed
wants to merge 1 commit into from

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Dec 26, 2018

Motivation for this change

1f6b095 has switched gtest to split shared build, but arrow-cpp assumes that headers are near the static libraries.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

1f6b095 has switched gtest to split shared
build, but arrow-cpp assumes that headers are near the static libraries.
orivej referenced this pull request Dec 26, 2018
Arch provides shared library as well.
@jtojnar
Copy link
Contributor

jtojnar commented Dec 26, 2018

Should not the gtest be added to buildInputs? So that FindGTest.cmake can discover it?

@jtojnar
Copy link
Contributor

jtojnar commented Dec 26, 2018

Hmm, even if I remove the split outputs, result/lib/cmake/GTest/GTestTargets.cmake will still be broken:

set_target_properties(GTest::gtest PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "GTEST_LINKED_AS_SHARED_LIBRARY=1"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}//nix/store/cg7qb5dch7gd6rgpxz2c00xvqwxb4lwc-gtest-1.8.1/include"
  INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}//nix/store/cg7qb5dch7gd6rgpxz2c00xvqwxb4lwc-gtest-1.8.1/include"
)

I have just recently fixed the same issue in openjpeg but they do not use generated export files like gtest does.

@orivej
Copy link
Contributor Author

orivej commented Dec 26, 2018

arrow-cpp has a custom FindGTest, it ignores cmake modules provided by gtest: https://github.com/apache/arrow/blob/apache-arrow-0.11.1/cpp/cmake_modules/FindGTest.cmake

I have just noticed that now gtest installs libgtest_main.so which can't be used as a main: gtest is right to assume that libgtest_main is static. EDIT: or may main of the program be defined in a shared library? EDIT2: it may.

@jtojnar
Copy link
Contributor

jtojnar commented Dec 26, 2018

I tried patching gtest and it now produces correct config files but the correct version is still not used, I am not sure where does the gtest 1.8.0 come from https://github.com/jtojnar/nixpkgs/pull/new/gtest

@orivej
Copy link
Contributor Author

orivej commented Dec 26, 2018

I don't know the exact origin of 1.8.0, but when GTEST_HOME is not set, arrow-cpp uses ExternalProject_Add(googletest_ep) instead of find_package(GTest): https://github.com/apache/arrow/blob/apache-arrow-0.11.1/cpp/cmake_modules/ThirdpartyToolchain.cmake#L489
This package build system is too convoluted. If our gtest does not match its expectations (static and not split), I'd let it use its own rather than patch it in Nixpkgs.

@veprbl
Copy link
Member

veprbl commented Dec 26, 2018

Using vendored builds will requires network access during the build and that's not allowed in restricted build on Hydra. You could try to implement download using fetchurl. The easier way is to just use symlinkJoin like it's done everywhere else in the expression.

@veprbl veprbl mentioned this pull request Dec 26, 2018
10 tasks
@orivej orivej closed this Dec 26, 2018
@orivej orivej deleted the arrow-cpp branch March 29, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants