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

fix: Support static builds #6

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Conversation

polarathene
Copy link
Contributor

@polarathene polarathene commented Sep 10, 2024

When trying to use cargo-zigbuild for --target x86_64-unknown-linux-musl from a glibc build host, it was not possible to make a static binary due to the isahc dependency only enabling static-curl by default.

Add a static feature to enable the isahc/static-ssl feature in addition to the default feature isahc/static-curl.

Not sure why, but it seems that static openssl also makes zlib static 🤔

Add a `static` feature to enable the `isahc/static-ssl` feature in addition to the default feature `isahc/static-curl`.
@ijagberg
Copy link
Owner

I have some plans for how to improve the client, I am not happy with having the model (measurements, fields etc.) in the same crate as the client. I will probably release a new version with a feature flag for the client itself, so that users can choose to only use the models. I can look into this PR in combination with that change.

@polarathene
Copy link
Contributor Author

This is a change for quickinstall usage of influx which I am not involved with, but appears to rely on measurements with the client too.

With the static build support from this PR the libz dependency would no longer need to be provided. Google distroless cc image doesn't have that, but could dynamic link anything else like glibc and OpenSSL. So would be about 16MB, while a full static build of the linked stats-server would be less than 8MB and could use Google distroless static image for the basic supplementary files like ca-certificates bundle bringing total image size down to 10MB.

Since cc is not presently viable nor static builds due to this crate, they currently use a debian-slim image with a total weight of 84MB (base image is 75MB.


I can look into this PR in combination with that change.

If this crate would continue to be the client, that would still require isahc for the curl client right? So would this PR require any changes?

From the sounds of it if you have measurements split out, that would be a separate dependency and feature?

If you're not familiar with the syntax, it would look something like this to support:

[dependencies]
influx_measurements = { version = "0.1.0", optional = true }

[features]
measurements = ["dep:influx_measurements"]

Optionally with a default = ["measurements"] feature which should avoid being a breaking change?

EDIT: Oops, I misread, you meant the other way with the client being optional.. Here you go:

[dependencies]
# This crate would have `isahc` and the `static` feature proposed by this PR:
influx_client = { version = "0.1.0", optional = true }

[features]
default = ["client"]
client = ["dep:influx_client"]
static = ["influx_client?/static"]

For reference:

I'm not sure if there's a better way to approach this feature drilling. It's a lengthy chain of deps to toggle the curl-sys / openssl-sys crates static features 😓


If you think the time to tackle that additional change might take some time, perhaps the approach with a static feature could still be merged first with a minor release prior?

It should be fairly seamless to then adopt the proposed adjustments shown above once you do split the client out.

@ijagberg
Copy link
Owner

Thanks for the very helpful comment! I'll try to merge your PR tonight and push a new minor version like you said.

And yes, the second approach you mentioned is essentially what I am planning. I am not particularly happy that I included a client in this library from the beginning, since it is just an extremely basic wrapper around an HTTP client with no real Influx-specific error handling or anything like that. So hiding the client mod behind a feature flag would at least make it opt-in and docs could recommend that users make their own client in a couple of lines of code instead.

@ijagberg ijagberg merged commit e933af0 into ijagberg:main Sep 11, 2024
1 check passed
@ijagberg
Copy link
Owner

I have published version 2.1.0 now, with your proposed changes. Let me know if it works. I didn't add any docs describing the feature since I am planning on releasing a new major version - I will improve the docs generally in that release.

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.

2 participants