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

tiledb: init at 1.6.3 #67187

Closed
wants to merge 1 commit into from
Closed

tiledb: init at 1.6.3 #67187

wants to merge 1 commit into from

Conversation

Rakesh4G
Copy link
Contributor

@Rakesh4G Rakesh4G commented Aug 21, 2019

Motivation for this change
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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @CMCDragonkai

@CMCDragonkai
Copy link
Member

We need help to figure out how to build the tiledb documentation. https://github.com/Intel-HLS/TileDB/wiki/Building-TileDB#step-8---build-tiledb and also how to run its gtest for the checkPhase. Anybody know how to do this?

@stavrospapadopoulos
Copy link

Just FYI, the official TileDB is maintained at https://github.com/TileDB-Inc/TileDB. We'd be happy to help with documentation and tests.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

essentially, i would do this:

Also, python and gtest (they use catch) aren't needed. They might be needed for the bindings, but for the base .so, you dont need it.

tiledb expression
{ stdenv, fetchFromGitHub
, boost
, bzip2
, catch2
, clang-tools
, cmake
, doxygen
, libpqxx
, lz4
, openssl
, spdlog_0
, tbb
, zlib
, zstd
}:

stdenv.mkDerivation rec {
  name = "tiledb";
  version = "1.5.1";

  src = fetchFromGitHub {
    owner = "TileDB-Inc";
    repo = "TileDB";
    rev = version;
    sha256 = "0ky0dcv1w1jn1cjn3819aq9xyd2wg80aagf2flxmd916flgr9zjl";
  };

  nativeBuildInputs = [ clang-tools cmake doxygen ];

  buildInputs = [ catch2 zlib lz4 bzip2 zstd spdlog_0 tbb openssl boost libpqxx ];

  # emulate the process of pulling catch down
  postPatch = ''
    mkdir -p build/externals/src/ep_catch
    ln -sf ${catch2}/include/catch2 build/externals/src/ep_catch/single_include
  '';

  makeTarget = "tiledb";
  installTargets = [ "-C tiledb install" ];

  meta = with stdenv.lib; {
    description = "TileDB allows you to manage the massive dense and sparse multi-dimensional array data";
    homepage = https://github.com/TileDB-Inc/TileDB;
    license = licenses.mit;
    maintainers = with maintainers; [ rakesh4g ];
  };
}


nativeBuildInputs = [clang-tools doxygen gtest];

buildInputs = [cmake catch2 zlib lz4 bzip2 zstd spdlog_0 tbb openssl boost libpqxx python ];
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace and move cmake to nativeBuildInputs. python could probably be moved too if it's not needed at runtime.

Suggested change
buildInputs = [cmake catch2 zlib lz4 bzip2 zstd spdlog_0 tbb openssl boost libpqxx python ];
buildInputs = [ catch2 zlib lz4 bzip2 zstd spdlog_0 tbb openssl boost libpqxx python ];


outputs = ["out"];

nativeBuildInputs = [clang-tools doxygen gtest];
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace, and add cmake

Suggested change
nativeBuildInputs = [clang-tools doxygen gtest];
nativeBuildInputs = [ clang-tools cmake doxygen gtest ];


makeTarget = "tiledb";

outputs = ["out"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a single output called "$out" is the default, you can remove

Suggested change
outputs = ["out"];

cmakeFlags = [
"-DCATCH_INCLUDE_DIR=${catch2}"
# "-DTILEDB_SUPERBUILD=OFF"
# "-DINCLUDE_DIR=${clang-tools}/bin"
Copy link
Contributor

Choose a reason for hiding this comment

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

let nix take care of setting those things like include, library, and other common dirs.

Copy link
Member

Choose a reason for hiding this comment

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

Which ones are needed and which ones are not?

@jonringer
Copy link
Contributor

We need help to figure out how to build the tiledb documentation. https://github.com/Intel-HLS/TileDB/wiki/Building-TileDB#step-8---build-tiledb and also how to run its gtest for the checkPhase. Anybody know how to do this?

for the docs:

preInstall = ''
  make doc
'';

for the gtest, no idea

@Rakesh4G
Copy link
Contributor Author

Rakesh4G commented Aug 22, 2019

We need help to figure out how to build the tiledb documentation. https://github.com/Intel-HLS/TileDB/wiki/Building-TileDB#step-8---build-tiledb and also how to run its gtest for the checkPhase. Anybody know how to do this?

for the docs:

preInstall = ''
  make doc
'';

for the gtest, no idea

Thanks @jonringer , Just used check in place of doc. It is triggering test build for tiledb. Though it is failing for other reasons.

@Rakesh4G
Copy link
Contributor Author

Just FYI, the official TileDB is maintained at https://github.com/TileDB-Inc/TileDB. We'd be happy to help with documentation and tests.

Thanks @stavrospapadopoulos ,

We have raised the issue at: https://github.com/TileDB-Inc/TileDB/issues/1369

@tdenniston
Copy link

Hello, Tyler from TileDB here. Unless there is a reason otherwise, we recommend you update your package to the latest stable release of TileDB, version 1.6.2. Now that major release 1.6 is available, bugfixes and support for older releases (such as 1.5.1) will be minimal.

@Rakesh4G
Copy link
Contributor Author

Rakesh4G commented Aug 22, 2019

@tdenniston Thanks for your response. We had initially tried with tiledb 1.6.2 only. But we were stuck at tiledb shared library build itself. We had to manually include map file in one or two files to build library itself.this is similar to TileDB-Inc/TileDB#1312 so we later moved back to version 1.5.1.

@tdenniston
Copy link

@Rakesh4G we'd like to help you get the 1.6.2 package building. Could you please tell us more information about the 1.6.2 build errors? We will investigate whether it is possible to do a 1.6.3 release to address your issues.

@CMCDragonkai
Copy link
Member

@tdenniston I think the errors he's referring to is here: TileDB-Inc/TileDB#1312

@Rakesh4G
Copy link
Contributor Author

Hi @tdenniston , Please also find attached the error log from my machine.
[ 51%] Building CXX object tiledb/CMakeFiles/TILEDB_CORE_OBJECTS.dir/sm/kv/kv.cc.o In file included from /build/TileDB-1208899/tiledb/../tiledb/sm/rtree/rtree.h:44:0, from /build/TileDB-1208899/tiledb/../tiledb/sm/fragment/fragment_metadata.h:41, from /build/TileDB-1208899/tiledb/sm/fragment/fragment_metadata.cc:34: /build/TileDB-1208899/tiledb/../tiledb/sm/subarray/subarray.h:529:8: error: 'map' in namespace 'std' does not name a template type std::map<std::vector<uint8_t>, size_t> tile_coords_map_; ^~~
tiledb-error_1.6.2.txt

Used Tiledb is :

   name = "tiledb";
    version = "1.6.2";
    url = "https://github.com/TileDB-Inc/TileDB";
    rev = "12088997fd4a72db6d9ee726c493e73ea38fb67d";
    sha256 = "sha256:0j0f2jmaxv4idywk3wxvziv69aqizi0z8kd54qi4rwnw2b6w3nsr";

Thanks.

@tdenniston
Copy link

@Rakesh4G if possible could you please try building the TileDB branch at https://github.com/TileDB-Inc/TileDB/tree/ttd/test-missing-include-fix? Commit hash 21279ea5a24cabc60d0ab6c75ad0e19db6004967. The branch is based off of TileDB 1.6.2 and if it fixes the build issue for you, we will include it in release 1.6.3.

@Rakesh4G
Copy link
Contributor Author

Hi @tdenniston , After using the patch now we are reaching a point where tiledb library is building. But the build error for tiledb_unit is same as before. please see the attached file
tiledb_unit-error_1.6.2_patch.txt

/build/TileDB-21279ea/test/src/unit-capi-consolidation.cc:2946:7: error: unused variable 'has_key' [-Werror=unused-variable]
   int has_key;
       ^~~~~~~
cc1plus: all warnings being treated as errors
make[7]: *** [test/CMakeFiles/tiledb_unit.dir/build.make:193: test/CMakeFiles/tiledb_unit.dir/src/unit-capi-consolidation.cc.o] Error 1

Thanks.

@tdenniston
Copy link

@Rakesh4G I've pushed a new commit to the same branch (hash 45e7e874570791a23d9c7586f6c08853daaa548c) that should fix the unit test build errors.

@Rakesh4G
Copy link
Contributor Author

Hi @tdenniston , Thanks. with this now it moves ahead but stops again in

unit-capi-sparse_array.cc:1958:15: error: unused variable 'attributes' [-Werror=unused-variable] const char* attributes[] = {"a2", TILEDB_COORDS};

[ 68%] Building CXX object test/CMakeFiles/tiledb_unit.dir/src/unit-capi-sparse_array.cc.o /build/TileDB-45e7e87/test/src/unit-capi-sparse_array.cc: In member function 'void SparseArrayFx::check_invalid_offsets(const string&)': /build/TileDB-45e7e87/test/src/unit-capi-sparse_array.cc:1958:15: error: unused variable 'attributes' [-Werror=unused-variable] const char* attributes[] = {"a2", TILEDB_COORDS}; ^~~~~~~~~~ cc1plus: all warnings being treated as errors make[7]: *** [test/CMakeFiles/tiledb_unit.dir/build.make:375: test/CMakeFiles/tiledb_unit.dir/src/unit-capi-sparse_array.cc.o] Error 1 make[6]: *** [CMakeFiles/Makefile2:2207: test/CMakeFiles/tiledb_unit.dir/all] Error 2 make[5]: *** [CMakeFiles/Makefile2:2182: test/CMakeFiles/check.dir/rule] Error 2 make[4]: *** [Makefile:942: check] Error 2 make[3]: *** [CMakeFiles/check.dir/build.make:57: CMakeFiles/check] Error 2 make[2]: *** [CMakeFiles/Makefile2:174: CMakeFiles/check.dir/all] Error 2 make[1]: *** [CMakeFiles/Makefile2:181: CMakeFiles/check.dir/rule] Error 2 make: *** [Makefile:157: check] Error 2 builder for '/nix/store/k2jnzv30c6yvrmzc1djbszh92qi4a6mf-tiledb.drv' failed with exit code 2 error: build of '/nix/store/k2jnzv30c6yvrmzc1djbszh92qi4a6mf-tiledb.drv' failed

@tdenniston
Copy link

@Rakesh4G I've pushed another update. Would you mind telling me your toolchain version and compiler flags? We have CI checks in place to prevent these errors, but our CI did not find these unused variables.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Aug 27, 2019

The toolchain would be whatever is supplied by the standard environment in Nix. So that would be the current master's gcc/clang? Nix is pretty strict for its compiler flags by default.

@jonringer
Copy link
Contributor

@tdenniston full configure log when I attempt to build this

cmake flags: -DCMAKE_FIND_PACKAGE_NO_SYSTEM_PACKAGE_REGISTRY=ON -DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_SKIP_BUILD_RPATH=ON -DBUILD_TESTING=OFF -DCMAKE_INSTALL_LOCALEDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/share/locale -DCMAKE_INSTALL_LIBEXECDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/libexec -DCMAKE_INSTALL_LIBDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/lib -DCMAKE_INSTALL_DOCDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/share/doc/ -DCMAKE_INSTALL_INFODIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/share/info -DCMAKE_INSTALL_MANDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/share/man -DCMAKE_INSTALL_OLDINCLUDEDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/include -DCMAKE_INSTALL_INCLUDEDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/include -DCMAKE_INSTALL_SBINDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/sbin -DCMAKE_INSTALL_BINDIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/bin -DCMAKE_INSTALL_NAME_DIR=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb/lib -DCMAKE_POLICY_DEFAULT_CMP0025=NEW -DCMAKE_OSX_DEPLOYMENT_TARGET= -DCMAKE_OSX_SYSROOT= -DCMAKE_FIND_FRAMEWORK=last -DCMAKE_STRIP=/nix/store/mgdjnsrkqgmxqjaqaxgqyqm7fwyi96fk-binutils-2.31.1/bin/strip -DCMAKE_RANLIB=/nix/store/mgdjnsrkqgmxqjaqaxgqyqm7fwyi96fk-binutils-2.31.1/bin/ranlib -DCMAKE_AR=/nix/store/mgdjnsrkqgmxqjaqaxgqyqm7fwyi96fk-binutils-2.31.1/bin/ar -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_INSTALL_PREFIX=/nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb
-- The C compiler identification is GNU 7.4.0
-- The CXX compiler identification is GNU 7.4.0
-- Check for working C compiler: /nix/store/hpzj855nkgjvg58nrhq4910sb9q3kss1-gcc-wrapper-7.4.0/bin/gcc
-- Check for working C compiler: /nix/store/hpzj855nkgjvg58nrhq4910sb9q3kss1-gcc-wrapper-7.4.0/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /nix/store/hpzj855nkgjvg58nrhq4910sb9q3kss1-gcc-wrapper-7.4.0/bin/g++
-- Check for working CXX compiler: /nix/store/hpzj855nkgjvg58nrhq4910sb9q3kss1-gcc-wrapper-7.4.0/bin/g++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Starting TileDB superbuild.
-- Found Bzip2: /nix/store/g8al6vdf21mgfi5774qx6r6cil1j7zr8-bzip2-1.0.6.0.1/lib/libbz2.so
-- Found LZ4: /nix/store/6s41b1kn253k4b8dwlyai76d1cazxcdm-lz4-1.9.1/lib/liblz4.so
-- Found Spdlog: /nix/store/3yw2kclpsx02b9hx6ssbbrwlixf5pdi5-spdlog-0.17.0/include
-- Found Zlib: /nix/store/w3wqang215is14xqajycbxmd52b44qkw-zlib-1.2.11/lib/libz.so
-- Found Zstd: /nix/store/aydp0jd0yvg5r5ynsqv1b6i67w736afv-zstd-1.4.1/lib/libzstd.so
-- Found OpenSSL: /nix/store/1g9n3mxrawawa5p9mv8g2baaxjvdg2ag-openssl-1.0.2s/lib/libssl.so /nix/store/1g9n3mxrawawa5p9mv8g2baaxjvdg2ag-openssl-1.0.2s/lib/libcrypto.so
-- Found TBB: /nix/store/g7p34bn41bzmjfi1kabkks8md9bdjx1p-tbb-2019_U8/lib/libtbb.so
-- Found TBB imported target: TBB::tbb
-- searching for catch in /build/TileDB-7236bb2/build/externals/src
-- Found Catch: /build/TileDB-7236bb2/build/externals/src/ep_catch/single_include
-- Not found clang-tidy
-- Not found clang-format
-- Found Doxygen: /nix/store/4b4pxa2l331s2z1qd3wh50a30nw22swn-doxygen-1.8.15/bin/doxygen (found version "1.8.15") found components:  doxygen missing components:  dot
-- Install prefix is /nix/store/7hgm4n1iprbdlpmv1nhvzbd4kk9g67kv-tiledb
-- Configuring done
-- Generating done

@Rakesh4G
Copy link
Contributor Author

Rakesh4G commented Aug 27, 2019

Hi @tdenniston , Thanks again. it moved further but still failing at:

[ 90%] Building CXX object test/CMakeFiles/tiledb_unit.dir/src/unit-vfs.cc.o /build/TileDB-2958fb5/test/src/unit-vfs.cc: In function 'void ____C_A_T_C_H____T_E_S_T____0()': /build/TileDB-2958fb5/test/src/unit-vfs.cc:118:20: error: unused variable 'batch_bytes' [-Werror=unused-variable] const unsigned batch_bytes = 13; ^~~~~~~~~~~ /build/TileDB-2958fb5/test/src/unit-vfs.cc:176:20: error: unused variable 'batch_bytes' [-Werror=unused-variable] const unsigned batch_bytes = 13; ^~~~~~~~~~~ /build/TileDB-2958fb5/test/src/unit-vfs.cc:200:20: error: unused variable 'batch_bytes' [-Werror=unused-variable] const unsigned batch_bytes = 13; ^~~~~~~~~~~ cc1plus: all warnings being treated as errors make[7]: *** [test/CMakeFiles/tiledb_unit.dir/build.make:817: test/CMakeFiles/tiledb_unit.dir/src/unit-vfs.cc.o] Error 1

Also i had tried to do this change on my local branch forked from your branch to see if any other issue is still there. The result is tiledb_unit build is successful after above mentioned change.

Now during execution my test run is failing for few test cases Full log -->
tiledb_unit_run_error.txt
:

===================================================================
test cases: 371 | 365 passed | 6 failed
assertions: 1508424 | 1508163 passed | 261 failed
0% tests passed, 1 tests failed out of 1

Total Test time (real) = 19.06 sec

The following tests FAILED:
1 - tiledb_unit (Failed)
Errors while running CTest

I assume mostof them are failing due to same issue :
/build/TileDB-b95b40d/test/src/unit-capi-consolidation.cc:1883: FAILED:
CHECK( a[i] == c_a[i] ) with expansion: -2147483648 == 315 (0x13b)

@Rakesh4G Rakesh4G marked this pull request as ready for review August 27, 2019 05:03
@tdenniston
Copy link

@Rakesh4G I've fixed the last unused variable warning on my branch.

For the test failures, can you tell us more about your environment? We are not familiar with Nix in general -- what hardware architecture are you building on?

@jonringer
Copy link
Contributor

jonringer commented Aug 27, 2019

@tdenniston if you want to experiment locally on a linux box, you can follow these steps:

git clone git@github.com:NixOS/nixpkgs.git
cd nixpkgs
git fetch pull/67187/head:tiledb   # fetch this PR and assign it to the branch  tiledb
git checkout tiledb
nix-build -A tiledb

nixpkgs will pull down or build all of your dependencies, then attempt to build the tiledb package.

@jonringer
Copy link
Contributor

I think you pushed the wrong branch

@Rakesh4G
Copy link
Contributor Author

I think you pushed the wrong branch

Yes, I was trying to squash and pushed something wrong. I will correct it soon.

@FRidh
Copy link
Member

FRidh commented Nov 25, 2019

pushed 1.7.0 in df13589 and the python bindings in 6e61ff0

@FRidh FRidh closed this Nov 25, 2019
@ihnorton
Copy link

That's great, thanks all who pushed this through. Please open an issue or ping us at hello <at> tiledb.com in future if needed.

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

8 participants