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

CI for CLI xplat binary build, connector definition build, CLI plugin manifest generation, GitHub release #4

Merged
merged 8 commits into from
Mar 18, 2024

Conversation

daniel-chambers
Copy link
Contributor

@daniel-chambers daniel-chambers commented Mar 15, 2024

This PR adds CI steps for:

  • Building the CLI plugin binaries across multiple platforms (Mac/Windows/Linux ARM64/AMD64)
    • This is done using a rustup toolchain across a matrix build in GitHub actions (ie. not using Nix cross compilation) and is stolen from ndc-postgres's implementation
  • Building the connector definition tarfile
    • This incorporates the built docker image name and tag, as well as the CLI plugin version
  • Generating the CLI plugin manifest
    • This is kept in the build artifacts, for now. When a release is performed it will need to be manually contributed to the cli-plugin-index repo. We can try to automate this when we're confident in the stability of the release process
    • This manifest contains URLs to downloads of the CLI binaries from the GitHub release (see below) as well as SHA256 hashes of the binaries
  • Creating a draft GitHub release from the CLI binaries and connector definition, with the changelog extracted from CHANGELOG.md

The GitHub Release will look something like this:
image

The new CHANGELOG.md file should contain our changes and should be updated before the release commit is tagged. It uses
Keep A Changelog format, which means it can be parsed and used during the build to populate the text in the release.

Other changes:

  • Got rid of the custom binary name for the cli plugin; the binaries are renamed in the plugin manifest.yaml to hasura-mongodb, and the crate name provides a better name that properly indicates what the executable is for, which is important since the binaries show up in the GitHub release
  • Fixed deploy.sh and docker.nix using the wrong container image name

Issue ticket number and link

JIRA: MDB-79

@hallettj
Copy link
Collaborator

I forgot, the binaries should be statically linked. You can see how I did that in cargo-boilerplate.nix, with some openssl-specific requirements in mongodb-connector-workspace.nix

Copy link
Collaborator

@hallettj hallettj left a comment

Choose a reason for hiding this comment

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

Looks good!

I mentioned statically-linking binaries, but come to think of it I only know how to do that for linux binaries at the moment. The sticking point is openssl which makes itself tricky to statically-link. It might be easiest to switch to a different ssl implementation to get static linking.

@daniel-chambers
Copy link
Contributor Author

@hallettj I don't think we need to worry about static linking here. The CLI does not consume OpenSSL at all, because cross compiling that on ARM didn't work, so I got @dmoverton to move things around to remove the dependency on ndc-sdk (which was bringing in OpenSSL transitively) from the CLI to avoid bringing in OpenSSL. ndc-postgres appear to have done the same thing, which is what gave me that idea. 😄

@hallettj
Copy link
Collaborator

@daniel-chambers We probably still need to think about static linking since there are other library dependencies:

❯ ldd target/release/hasura-mongodb
	linux-vdso.so.1 (0x00007ffc64937000)
	libgcc_s.so.1 => /nix/store/pp0jsd045xvfsz60kpbkfxbs9pbpk8z5-gcc-13.2.0-lib/lib/libgcc_s.so.1 (0x00007f42850b2000)
	libm.so.6 => /nix/store/ksk3rnb0ljx8gngzk19jlmbjyvac4hw6-glibc-2.38-44/lib/libm.so.6 (0x00007f4284fd0000)
	libc.so.6 => /nix/store/ksk3rnb0ljx8gngzk19jlmbjyvac4hw6-glibc-2.38-44/lib/libc.so.6 (0x00007f4284417000)
	/nix/store/ksk3rnb0ljx8gngzk19jlmbjyvac4hw6-glibc-2.38-44/lib/ld-linux-x86-64.so.2 => /nix/store/ksk3rnb0ljx8gngzk19jlmbjyvac4hw6-glibc-2.38-44/lib64/ld-linux-x86-64.so.2 (0x00007f42850d9000)

The issue with openssl is that it's extra-hard to statically link so I was using musl instead of glibc, but musl is linux-only. Without openssl static linking should be easier.

@daniel-chambers
Copy link
Contributor Author

@hallettj My understanding is that it is highly recommended to not statically link glibc (it is unsupported) and so we'd have to use musl if we wanted to do static linking... I guess we'd have to figure out static linking on other platforms separately?

@daniel-chambers
Copy link
Contributor Author

@hallettj OK, I've enabled static linking for Linux and Windows binaries. On Linux we use musl as the libc, and on Windows we statically link the MSVC libs (I can use dumpbin /imports ... to see that VCRUNTIME140.dll is no longer dynamically imported).

I have zero idea what to do with Mac stuff; I might leave that as is until someone complains and fixes it themselves. 😅

@hallettj
Copy link
Collaborator

Good point, we're not going to statically link glibc. But I think I read that you can statically link everything else. And then hopefully the host system has a compatible libc installed already.

@daniel-chambers
Copy link
Contributor Author

@hallettj Well, I've gone and just statically linked the entire thing against musl. 😅 Seems legit.

@hallettj
Copy link
Collaborator

@hallettj OK, I've enabled static linking for Linux and Windows binaries. On Linux we use musl as the libc, and on Windows we statically link the MSVC libs (I can use dumpbin /imports ... to see that VCRUNTIME140.dll is no longer dynamically imported).

Very nice!

I have zero idea what to do with Mac stuff; I might leave that as is until someone complains and fixes it themselves. 😅

Seems reasonable to me

@daniel-chambers daniel-chambers merged commit 36f435c into main Mar 18, 2024
2 checks passed
@daniel-chambers daniel-chambers deleted the daniel/packaging branch March 18, 2024 21:50
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