Single release CMake file that contains all required links/hashes. #4631

dudoslav · 2024-01-15T11:46:20Z

This PR aims to autogenerate cmake file containing all required hash/url combinations so that other repositories can simply pull this file and forget about changing all url/hash occurences in their code. The output assets look like this now:

https://github.com/dudoslav/TileDB/releases/tag/t01

Please let me know if the produced CMake file is ok, or should be changed to a different format.

TYPE: BUILD
DESC: Single release CMake file that contains all required links/hashes.

shortcut-integration · 2024-01-15T11:46:23Z

This pull request has been linked to Shortcut Story #17775: Generate cmake file with artifact URLs and checksums for each release.

ihnorton

Thanks, this is a great start that will simplify updates for downstream packages.

I am hesitant to have a tight coupling between all of FindTileDB_EP.cmake and downstream packages. For now I think we should just provide the capability to get TileDB from github releases, but not aim to have users replace the whole FindTileDB_EP.cmake (in case there are package-specific modifications, etc.)

I think we should limit this to a .cmake file with:

package URLs and SHAs
create a CMake function for the code here, and include it in the file. Downstream packages can call the function inside of their own FindTileDB_EP.cmake.

See comment inline about using cmake configure_file to create this from a .in template.

ihnorton · 2024-01-15T12:07:17Z

.github/misc/FindTileDB_EP.cmake

@@ -0,0 +1,141 @@
+#


Let's call this TileDB-<version>-packages.cmake.in, and use cmake configure file to do the substitution: https://cmake.org/cmake/help/latest/command/configure_file.html

Also, it can live in https://github.com/TileDB-Inc/TileDB/tree/dev/cmake/inputs

Hmm, I might not understand properly what you are trying to achieve. configure_file(...) can be called from within another CMake file, so we would need something like generate.cmake and TileDB-<version>-packages.cmake.in that would be used to generate TileDB-<version>-packages.cmake. In any case we still need to substitute all values in BASH script since it has the whole list-regex logic. Is this what you had in mind?:

GitHub CI -> creates generate.cmake with all required values already in it.

generate.cmake uses these values to configure_file TileDB-.....cmake.in (This could have been done by CI in the first step)

upload the produced TileDB-...cmake with its SHA to release (We don't need to upload generate.cmake)

Do I understand it correctly?
If not could you perhaps explain your idea a bit more?

eddelbuettel · 2024-01-15T13:37:08Z

Love this. Having the information in one place (by release) is very good. But may I raise my hand and ask for a (complementary) non-cmake format? Maybe JSON? Maybe key:value pairs? R does use cmake and while I could shimmy it (with help) a simpler approach would be good too.

dudoslav · 2024-01-15T16:02:39Z

I have no problem also adding a json format, however, do you really need release build outside cmake?

I can read JSON from CMAKE without any issues since I can use string(JSON ...) functions

ihnorton · 2024-01-15T16:21:26Z

Yes, good point @eddelbuettel

I have no problem also adding a json format, however, do you really need release build outside cmake?

Yes, these builds are used by TileDB-R outside of CMake.

eddelbuettel · 2024-01-15T16:26:41Z

To be perfectly plain, I would prefer non-JSON and reading JSON happens to not be batteries included. I currently use the 'Debian Control File' format R itself uses in its DESCRIPTION file, for that we have a ready-made parser included (read.dcf() that is):

https://github.com/TileDB-Inc/TileDB-R/blob/e17563a554921d952ac473d5e49b65888a6a035a/tools/tiledbVersion.txt#L1-L2

Of course, other formats work but at the cost of added dependencies. So JSON, YAML, TOML, ... are all equally (in)convenient with the need for other library.

eddelbuettel · 2024-01-15T16:29:41Z

do you really need release build outside cmake?

Yeah. Let's plan on a quick huddle this week and I show you a quick demo and more resources. In essence, R is driven by e.g. R CMD INSTALL nameOfArchve_x.y.z.tar.gz and invokes just make and other standard tools (via pre-made added Makefile snippets). One can farm out to cmake and other callable tools but it counts as extra dependency. We currently only depend on cmake if a build from (full, ie TileDB) source is requested else R and shell etc lead to the download of an artifact.

ihnorton · 2024-01-15T16:39:16Z

cmake/inputs/TileDB-packages.cmake.in

+
+function(DOWNLOAD_TILEDB FROM_SOURCE)
+    # Available builds
+    set(URL_TILEDB_LINUX_X86_64 "{{URL_TILEDB_LINUX_X86_64}}")


I think we should set these in PARENT_SCOPE

I didn't want to pollute the parent scope with these variables, since noone else needs them

teo-tsirpanis

Two quick comments, I will need some time to think of this design.

teo-tsirpanis · 2024-01-15T17:40:04Z

cmake/inputs/TileDB-packages.cmake.in

+        ExternalProject_Add(ep_tiledb
+                PREFIX "externals"
+                URL "${URL_TILEDB_SOURCE}"
+                URL_HASH SHA1=${SHA_TILEDB_SOURCE}


SHA-1 is insecure; we should be using at least SHA-256.

I agree, I would like to change it if everyone is OK with it

teo-tsirpanis · 2024-01-15T17:41:01Z

cmake/inputs/TileDB-packages.cmake.in

+                CMAKE_ARGS
+                -DCMAKE_INSTALL_PREFIX=${EP_INSTALL_PREFIX}
+                -DCMAKE_PREFIX_PATH=${EP_INSTALL_PREFIX}
+                -DTILEDB_S3=${TILEDB_S3}


We should be passing more options here. The available set of options is at BuildOptions.cmake. I'm not saying to pass all of them, but the user should be able to set some more like Azure, GCS and WebP.

We should add these options to function's arguments

dudoslav · 2024-01-16T09:31:38Z

Ok so my next plan is to:

Create CSV file of all releases + source
Change sha1 to sha256 if its ok with everyone (because it might create more problems downstream)
Modify cmake to use this csv file
Divide cmake function into download_prebuilt and download_source

dudoslav · 2024-01-16T10:07:53Z

This might be a format that could be compatible with both R and CMake:

(base) dudoslav@beetle:~$ cat rellist.csv
os,arch,url,sha,noavx2
LINUX,X86_64,https://SOMEURL.com,da39a3ee5e6b4b0d3255bfef95601890afd80709,
LINUX,X86_64,https://SOMEURL.com,da39a3ee5e6b4b0d3255bfef95601890afd80709,NOAVX2
WINDOWS,X86_64,https://SOMEURL.com,da39a3ee5e6b4b0d3255bfef95601890afd80709,

teo-tsirpanis · 2024-01-16T11:13:24Z

DESC: Look above

The item in the DESC field will be included in the release notes. You should set it to something meaningful like Add redistributable CMake file that downloads prebuilt binaries from GitHub Releases.

teo-tsirpanis · 2024-01-16T12:32:59Z

Here's how I am imagining this. Each release will have a JSON (or another format like CSV but I would prefer JSON IMHO) file named say artifacts.json that will contain information about the available release artifacts. Here's an example for 2.19.0:

{
    // We could add this to help non-vcpkg builds from source.
    "git-ref": "fa30a88abc5504ed8387e25937744afcd3802232",
    "artifacts": {
        "x64-windows": {
            "filename": "tiledb-windows-x86_64-2.19.0-fa30a88a.zip",
            "sha512": "…"
        },
        "x64-linux": {
            "filename": "tiledb-linux-x86_64-2.19.0-fa30a88a.zip",
            "sha512": "…"
        },
        "x64-linux-noavx2": {
            "filename": "tiledb-linux-x86_64-noavx2-2.19.0-fa30a88a.zip",
            "sha512": "…"
        },
        "x64-macos": {
            "filename": "tiledb-macos-x86_64-2.19.0-fa30a88a.zip",
            "sha512": "…"
        },
        "arm64-macos": {
            "filename": "tiledb-macos-arm64-2.19.0-fa30a88a.zip",
            "sha512": "…"
        }
    }
}

By reading from the JSON file, the CMake script can be version-agnostic and be checked in once for every downstream repository. Here's how the script could be used:

include(DownloadPrebuiltTileDB)

# Downloads prebuilt binaries of TileDB from GitHub
# Releases and calls find_package to import them.
find_prebuilt_tiledb(
  # The version of TileDB to use. Required.
  VERSION 2.19.0
  # The hash of the artifacts.json file. Optional, will warn if not
  # specified, maybe suggesting to at least enable CMAKE_TLS_VERIFY.
  MANIFEST_SHA512 …
  # The name of the release artifact to download.
  # Optional, will try to autodetect if not specified.
  ARTIFACT_NAME x64-linux-noavx2
  # Flags that will also be passed to the find_package call.
  REQUIRED QUIET
)

target_link_libraries(my_project PRIVATE TileDB::tiledb)

Regarding building from source I am wondering if it would be a better idea to have this CMake script exclusively deal with downloading prebuilt binaries, and direct users that want more customizability to vcpkg (after we create a tiledb port, which can be done at any time). Scoping this out will significantly simplify the script.

dudoslav · 2024-01-16T12:52:37Z

@teo-tsirpanis I really like your suggestion, but there is a requirement from the R team for a format that could be easily included without dependencies. So JSON is somewhat out of question. Other than that, the CMake script will contain two functions:

fetch_release_list that downloads and parses the release list (rellist.csv)
download_source_tiledb that downloads and build source tiledb
download_prebuilt_tiledb that downloads prebuilt tiledb

The question is that if we don't need download_source_tiledb then I could get rid of fetch_release_list function. However, are we really sure we don't need that source tiledb distribution?

eddelbuettel · 2024-01-16T13:18:56Z

The R build would need to load another package just to parse JSON, there is no built-in reader. Csv and alike work fine.

Another idea, because we are not short those few bytes, would be to store both sha1 and sha256 checksums.

dudoslav · 2024-01-16T13:39:48Z

Also, I am still not sure where to put this download logic, since it will not change between releases. Only the rellist changes. Maybe if I would hardcode the latest version into it it could be also distributed as release asset otherwise I am not sure what to do with it

dudoslav · 2024-01-16T13:51:59Z

Ok, so what do you guys think about the latest changes and mostly about the DownloadPrebuiltTileDB.cmake file? I removed the source distribution logic as requested and add parameters to that CMake script.

Edit 1.: As always the produced CSV can be found here: https://github.com/dudoslav/TileDB/releases/download/t01/rellist.csv

Edit 2: Fixed link

Edit 2: Change link directly to CSV

eddelbuettel · 2024-01-16T14:16:04Z

As always the produced CSV can be found here:

That's a cmake file, not a csv file.

Edit two weeks later: Not sure what I was up to in that comment. Hm.

dudoslav · 2024-02-05T15:00:44Z

UPDATE: I had to rebase this branch on top of another branch that add CPACK releases, since those ones actually generate sha256 hashes. This means that this PR should be merged after that change: #4694

Here are proposed release assets:
https://github.com/dudoslav/TileDB/releases/tag/t03

ihnorton

💯

teo-tsirpanis · 2024-02-26T11:21:37Z

cmake/inputs/DownloadPrebuiltTileDB.cmake

+                detect_artifact_name(FETCH_PREBUILT_TILEDB_ARTIFACT_NAME)
+        endif()
+
+        ExternalProject_Add(ep_tiledb


Use FetchContent or plain downloading?

I don't think that this PR should rework this download logic from the original approach.

I am concerned that ExternalProject might force downstream projects to use a superbuild architecture.

ExternalProject

Is this something we should address now in your opinion?

Hmm, the issue is that they still use it right? Don't we want to switch to vcpkg style dependencies? (ports)

I think the plan is that all first-party downstream projects eventually get TileDB from vcpkg.

We might want to change ExternalProject later, but it will be a breaking change which might or might not be OK to do.

On second thought the use of ExternalProject in this case will not require a superbuild project because we are using pre-built artifacts. I am not opposed to ExternalProject then. There is still the question of what would happen with using this script to download source artifacts.

teo-tsirpanis · 2024-02-26T11:25:10Z

cmake/inputs/DownloadPrebuiltTileDB.cmake

+                list(GET LINE 0 OS)
+                list(GET LINE 1 ARCH)
+                list(GET LINE 2 URL)
+                list(GET LINE 3 SHA)
+
+                if(${LENGTH} GREATER 4)
+                        list(GET LINE 4 NOAVX2)
+                endif()


I still have some reservations on how good this schema is. A more scalable and clean solution would be a PLATFORM,URL,SHA, where PLATFORM would be a string like x64-windows, x64-linux-noavx2, or source.

I have no problem reworking it that way, I assume that it would work for everyone.

teo-tsirpanis · 2024-03-01T12:06:30Z

cmake/inputs/DownloadPrebuiltTileDB.cmake

@@ -74,7 +74,7 @@ endfunction()

 function(detect_artifact_name OUT_VAR)
        if (WIN32) # Windows
-                SET(${OUT_VAR} TILEDB_WINDOWS_X86_64 PARENT_SCOPE)
+                SET(${OUT_VAR} TILEDB_WINDOWS-X86_64 PARENT_SCOPE)


We should fail on other architectures. Same in Linux.

I think that CMAKE can only define these three variables: WIN32/APPLE/LINUX

dudoslav · 2024-03-01T12:29:57Z

I changed download logic to use FetchContent and add ReleaseListGeneration into Release workflow. This is the resulting artifacts:

https://github.com/dudoslav/TileDB/releases/tag/t09

And this is the pipeline that produced it:

https://github.com/dudoslav/TileDB/actions/runs/8110756605

I also downloaded DownloadPrebuiltTileDB.cmake and tested in local dummy project.

dudoslav requested a review from ihnorton January 15, 2024 11:46

dudoslav self-assigned this Jan 15, 2024

ihnorton requested changes Jan 15, 2024

View reviewed changes

ihnorton reviewed Jan 15, 2024

View reviewed changes

teo-tsirpanis reviewed Jan 15, 2024

View reviewed changes

dudoslav added 10 commits January 31, 2024 11:33

Modify release.yml workflow

8c02c87

Don't build using superbuild

94d0194

Fix typo in binary package step

442f9f1

Change source package file name

4691c69

Fix artifact upload

6cf7c9c

Fix incorrect names for source and binary archive

efe3772

Use CPack in CI instead of CMake

e1cb7ca

Try packaging in Superbuild

0884182

Build before packaging

ded384c

Divide source and binary releases

c276d08

Rebase and fix for CPack logic

705260d

dudoslav force-pushed the db/sc-17775/release_cmake branch from 4a40e0c to 705260d Compare February 5, 2024 14:20

dudoslav added 7 commits February 5, 2024 15:24

Ignore HASH files

e8ac4cc

Expand source file name

8bcf35e

Fix filename expansion

448d0a8

Fix source file name path

04ed61f

Match release files and not source files

1fbcb02

Fix matching version

43a877d

Fix SHA256 HASH reading

881d79c

ihnorton approved these changes Feb 26, 2024

View reviewed changes

ihnorton requested a review from teo-tsirpanis February 26, 2024 03:02

Merge remote-tracking branch 'origin/dev' into db/sc-17775/release_cmake

1160c31

teo-tsirpanis reviewed Feb 26, 2024

View reviewed changes

dudoslav added 7 commits February 27, 2024 10:00

Merge remote-tracking branch 'origin/dev' into db/sc-17775/release_cmake

119107a

Rework rellist format

1209aa0

Automatically call generate release-list in release

8c38ab0

Fix workflow call

e36fe6a

Fix source file export

d710704

Add debugging urls

2253c38

Fix artifact assets download url

6d12f0f

ihnorton requested a review from teo-tsirpanis February 28, 2024 15:29

teo-tsirpanis approved these changes Feb 28, 2024

View reviewed changes

KiterLuc changed the title ~~Single release CMake file that contains all required links/hashes~~ Single release CMake file that contains all required links/hashes. Feb 28, 2024

dudoslav added 2 commits March 1, 2024 11:00

Use fetch content in DownloadPrebuiltTileDB

6d01260

Add ref_name to generate release list

c224bda

teo-tsirpanis reviewed Mar 1, 2024

View reviewed changes

teo-tsirpanis approved these changes Mar 1, 2024

View reviewed changes

KiterLuc merged commit 79030b1 into TileDB-Inc:dev Mar 4, 2024
55 checks passed

Single release CMake file that contains all required links/hashes. #4631

Single release CMake file that contains all required links/hashes. #4631

Conversation

dudoslav commented Jan 15, 2024 • edited by KiterLuc Loading

shortcut-integration bot commented Jan 15, 2024

ihnorton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddelbuettel commented Jan 15, 2024

dudoslav commented Jan 15, 2024 • edited Loading

ihnorton commented Jan 15, 2024

eddelbuettel commented Jan 15, 2024

eddelbuettel commented Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-tsirpanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dudoslav commented Jan 16, 2024

dudoslav commented Jan 16, 2024

teo-tsirpanis commented Jan 16, 2024

teo-tsirpanis commented Jan 16, 2024 • edited Loading

dudoslav commented Jan 16, 2024

eddelbuettel commented Jan 16, 2024

dudoslav commented Jan 16, 2024

dudoslav commented Jan 16, 2024 • edited Loading

eddelbuettel commented Jan 16, 2024 • edited Loading

dudoslav commented Feb 5, 2024

ihnorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-tsirpanis Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dudoslav commented Mar 1, 2024

dudoslav commented Jan 15, 2024 •

edited by KiterLuc

Loading

ihnorton left a comment •

edited

Loading

dudoslav commented Jan 15, 2024 •

edited

Loading

eddelbuettel commented Jan 15, 2024 •

edited

Loading

teo-tsirpanis commented Jan 16, 2024 •

edited

Loading

dudoslav commented Jan 16, 2024 •

edited

Loading

eddelbuettel commented Jan 16, 2024 •

edited

Loading

teo-tsirpanis Feb 27, 2024 •

edited

Loading