Skip to content

Remove the superbuild and the external projects. #5021

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

Merged
merged 26 commits into from
Jul 17, 2024
Merged

Conversation

teo-tsirpanis
Copy link
Member

@teo-tsirpanis teo-tsirpanis commented May 29, 2024

SC-36912
SC-36913

Historically, the Core's build system has been using CMake external projects to download and build external dependencies, and a "superbuild" architecture to ensure a build order. With the advent of vcpkg, we have stopped building the dependencies ourselves and instead rely on vcpkg (or the "system" in general) to provide them for us. The superbuild has thus became a relic of the past, consisting of only the inner tiledb project when the new system is enabled (formerly by specifying -DTILEDB_VCPKG=ON, now always).

This PR removes the superbuild. TileDB became a regular CMake project, whose targets can be built directly without first building the outer project, and then building the build/tiledb subdirectory. The CI scripts were updated accordingly to not use the subdirectory.

This is inevitably a breaking change in the build system. For starters, local development environment will certainly need to make a clean build after this change. Furthermore, there will need to be changes in build scripts to not build again on the tiledb subdirectory.

For examples of downstream migrations, TileDB-Inc/TileDB-Go#316 uses a make invocation that has a similar effect both with and without the superbuild, and conda-forge/tiledb-feedstock#290 uses a semi-documented CMake option to disable the superbuild (which will have no effect after the superbuild gets removed).

The majority of first-party downstreams (VCF, SOMA, MariaDB, Vector Search, the Python and Java APIs) use the install-tiledb target, which currently is defiend on the superbuild and builds the install target in the inner tiledb project. With this PR the install-tiledb target will be kept for compatibility, but alias to install.

I tried to build VCF with a TileDB external project from this branch, but it fails with an error that will be fixed with #4989. I will try again once that PR gets merged. Never mind, VCF builds with the latest changes.


TYPE: BUILD
DESC: The superbuild architecture of the build system has been removed and TileDB is a regular CMake project. Build commands of the form make && make -C tiledb <targets> will have to be replaced by make <targets>.

@KiterLuc
Copy link
Contributor

Was the breaking change announced in any way?

@teo-tsirpanis
Copy link
Member Author

The change will be announced in the release notes. There is unfortunately no way to gradually introduce it (proposals to first make the superbuild off by default and then remove it were rejected), but at the same time the change is quite simple to migrate.

@teo-tsirpanis teo-tsirpanis force-pushed the teo/remove-superbuild branch from f25ee66 to 917b7cf Compare May 29, 2024 14:07
@teo-tsirpanis teo-tsirpanis force-pushed the teo/remove-superbuild branch 2 times, most recently from ae47237 to ceeb0f5 Compare May 29, 2024 14:15
@KiterLuc
Copy link
Contributor

The change will be announced in the release notes. There is unfortunately no way to gradually introduce it (proposals to first make the superbuild off by default and then remove it were rejected), but at the same time the change is quite simple to migrate.

We probably need to announce removal and then actually do the removal in a couple releases. We cannot ship a release that will break dependencies and then expect them to fix it.

@teo-tsirpanis teo-tsirpanis force-pushed the teo/remove-superbuild branch from ceeb0f5 to 0f54966 Compare May 31, 2024 01:20
@teo-tsirpanis

This comment was marked as resolved.

@teo-tsirpanis teo-tsirpanis marked this pull request as ready for review May 31, 2024 18:00
@teo-tsirpanis
Copy link
Member Author

Undrafting, regardless of when we will take it, it is finished and ready for review.


Also just built successfully1 libtiledbsoma with just the following change:

diff --git a/libtiledbsoma/cmake/Modules/FindTileDB_EP.cmake b/libtiledbsoma/cmake/Modules/FindTileDB_EP.cmake
index 910fc4e..62bd32a 100644
--- a/libtiledbsoma/cmake/Modules/FindTileDB_EP.cmake
+++ b/libtiledbsoma/cmake/Modules/FindTileDB_EP.cmake
@@ -113,8 +113,8 @@ else()
     else() # Build from source
         ExternalProject_Add(ep_tiledb
           PREFIX "externals"
-          URL "https://github.com/TileDB-Inc/TileDB/archive/2.23.0.zip"
-          URL_HASH SHA1=2acca37618f0a6192ca648930138231476422b96
+          GIT_REPOSITORY "https://github.com/TileDB-Inc/TileDB.git"
+          GIT_TAG teo/remove-superbuild
           DOWNLOAD_NAME "tiledb.zip"
           CMAKE_ARGS
             -DCMAKE_INSTALL_PREFIX=${EP_INSTALL_PREFIX}

Footnotes

  1. With some compile errors because Windows, but configuring has been successful.

With vcpkg this step is unnecessary.
There is little value in doing this, and the configuring with tests disabled is tested in the release worklow.
It handles variations in the target name, and the possibility that no CMake package is available (like in Conda, which should make that Conda patch unnecessary).
It uses the unified `zstd::libzstd` target if available, otherwise a generator expression that chooses between the shared or the static library (in vcpkg only one of them will be available).
@teo-tsirpanis teo-tsirpanis force-pushed the teo/remove-superbuild branch from e0683b3 to 8848cb0 Compare June 11, 2024 12:30
@teo-tsirpanis teo-tsirpanis force-pushed the teo/remove-superbuild branch from 7d6521d to 7f660dc Compare June 13, 2024 18:43
@teo-tsirpanis teo-tsirpanis force-pushed the teo/remove-superbuild branch from 89509c5 to 8d9b959 Compare July 3, 2024 14:08
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

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

LGTM

This change is going to break IDE setups. It should be announced very loudly when it merges.

@teo-tsirpanis
Copy link
Member Author

Macports build (on macos-14 at least) on this commit succeeds, after removing an outdated patch for lz4.

@KiterLuc KiterLuc merged commit 253d1c7 into dev Jul 17, 2024
64 checks passed
@KiterLuc KiterLuc deleted the teo/remove-superbuild branch July 17, 2024 15:36
@jmckenna
Copy link

@KiterLuc thanks for this work. Question: I want to point to external projects (with the dev branch) with CMake, however now it always defaults to pulling in vcpkg files, how can I instead point to my externally built projects? Meaning: with this dev branch, can I set something like -DTILEDB_VCPKG=OFF, and then set variables through CMake (such as ZLIB_INCLUDE_DIR) for zlib, lz4, zstd, webp, etc ?

thanks again.

(related to old ticket #4070 )

@teo-tsirpanis
Copy link
Member Author

You can pass -DTILEDB_DISABLE_AUTO_VCPKG=ON to disable auto-downloading of vcpkg, making you responsible to provide the dependencies of TileDB from whatever sources you want.

@jmckenna
Copy link

ah, thanks @teo-tsirpanis !

@jmckenna
Copy link

jmckenna commented Jul 29, 2024

@teo-tsirpanis I do see the option listed in BuildOptions.cmake, however even if I enable it, CMake still continues to download and build the vcpkg packages.

I wonder if it is because I do not set a CMAKE_TOOLCHAIN_FILE variable (I pass all of the includes manually by passing all of the _INCLUDE_DIR and _LIBRARY paths for libs through the cmake command). This works fine for my other 500+ libraries that I build. Hmm I wonder what is happening in the TileDB case...

@teo-tsirpanis
Copy link
Member Author

I cannot reproduce locally. Do you perhaps have the VCPKG_ROOT environment variable set?

@jmckenna
Copy link

I do not set VCPKG_ROOT, or touch anything to do with VCPKG, all of the libs are compiled manually by myself (on MSVC 2022).

@jmckenna
Copy link

Even when I pass TILEDB_DISABLE_AUTO_VCPKG , CMake states -- Running vcpkg install...

Could it be that if CMake cannot find one dependency, instead of throwing an error, it begins a VCPKG install ?

@teo-tsirpanis
Copy link
Member Author

Could it be that if CMake cannot find one dependency, instead of throwing an error, it begins a VCPKG install ?

Very unlikely, vcpkg inside the project() CMake command. If finding packages fails after that CMake itself will fake.

Can you share a repro for me to try?

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