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

Add CPack to CMakeLists.txt #4645

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Add CPack to CMakeLists.txt #4645

merged 10 commits into from
Jan 29, 2024

Conversation

dudoslav
Copy link
Collaborator

@dudoslav dudoslav commented Jan 17, 2024

Add CPack to CMakeLists.txt. Currently the binary package was tested an it looks like it works.

All default package exports are enabled for tiledb project so:
image

The superbuild's package command generates outputs based on the current platform (TGZ for Linux).

Usage:

  • Superbuild
    • make package invokes cpack inside of the EP tiledb.
  • Build
    • cpack for default behaviour or cpack -G <GENERATOR> for specific output
    • make package
    • make package_source

Example output from Superbuild's make package:

TGZ
CPack: Create package using TGZ
CPack: Install projects
CPack: - Run preinstall target for: TileDB
CPack: - Install project: TileDB []
CPack: Create package
CPack: - package: /mnt/c/Users/dbara/CLionProjects/TileDB/build_superbuild/tiledb/tiledb-linux-x86_64-2.20.0.tar.gz generated.
CPack: - checksum file: /mnt/c/Users/dbara/CLionProjects/TileDB/build_superbuild/tiledb/tiledb-linux-x86_64-2.20.0.tar.gz.sha1 generated.

TYPE: FEATURE
DESC: Add CPack to CMakeLists.txt

Copy link

This pull request has been linked to Shortcut Story #38163: Create cmake target for release binary artifacts.

@ihnorton ihnorton marked this pull request as draft January 17, 2024 22:49
Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

  1. Let's add an example package config (even if it only needs to contain the version for .tar.gz output?)
  2. Please describe what package targets supported/not-supported in the PR description. The only for sc-38163 is to support the current .tar.gz target (I think you need more config to correctly generate an RPM or NSIS, and we don't need those yet)
  3. Please add a 1-2 sentence for how to test this in the PR description (just cpack in the build dir?)

@dudoslav
Copy link
Collaborator Author

  1. Done
  2. I pasted all available package formats for all platforms. I believe that for example for linux by default packages: .sh, .tar.gz, .tar.Z are produced.
  3. Done, I improved the description

Here are my questions:

  1. Should we document this logic somewhere in code, if yes, where?
  2. Should I disable .sh and .tar.Z default generators for Linux? We only need .tar.gz and producing other packages takes time.
  3. Should I change the package name to the same name as produced by GitHub release? Currently it is: TileDB-2.20.0-Linux.tar.gz however the release looks like this: tiledb-linux-x86_64-2.19.0-fa30a88a.tar.gz. (If not I have to rename the package in CI/CD)
  4. Should this PR also change CI/CD so that the resulting package will be used in release? Or is that so called "scope bleed"

@dudoslav
Copy link
Collaborator Author

Last changes also address:

  1. On Superbuild's make package only TGZ or ZIP generators are used, depending on the OS, in order to decrease CI/CD time.
  2. The output package names are now in the same format (except for the commit hash). I would like to forgo the dependency on git in the cmake.

@dudoslav dudoslav changed the title Draft: Add CPack to CMakeLists.txt Add CPack to CMakeLists.txt Jan 19, 2024
@dudoslav dudoslav marked this pull request as ready for review January 19, 2024 12:36
@dudoslav dudoslav self-assigned this Jan 22, 2024
Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

LGTM, let's get a +1 from @teo-tsirpanis too.

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

I ran the package target on my machine and it succeeded and the files in the zip look mostly fine to me. However I noticed that the bin/include/lib directories in the zip are inside a tiledb-win64-amd64-2.20.0 folder, while in the GitHub Release artifacts they are in the root of the archive. This is a breaking change.

@dudoslav
Copy link
Collaborator Author

I ran the package target on my machine and it succeeded and the files in the zip look mostly fine to me. However I noticed that the bin/include/lib directories in the zip are inside a tiledb-win64-amd64-2.20.0 folder, while in the GitHub Release artifacts they are in the root of the archive. This is a breaking change.

Very good observation, should be fixed now. Thank you @teo-tsirpanis

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Thanks!

@ihnorton ihnorton merged commit cab7c28 into dev Jan 29, 2024
63 checks passed
@ihnorton ihnorton deleted the db/sc-38163/cpack branch January 29, 2024 18:38
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.

3 participants