alexcrichton / curl-rust

Rust bindings to libcurl
MIT License
1k stars 234 forks source link

Enable the upkeep_7_62_0 and poll_7_68_0 features in CI #414

Open darkprokoba opened 2 years ago

darkprokoba commented 2 years ago

Enable the upkeep_7_62_0 and poll_7_68_0 features in CI whenever the curl_static feature is enabled.

alexcrichton commented 2 years ago

This feels like it'd be a bit of a bummer to maintain, I'd prefer to get a better story for enabling functionality found in later verisons of curl. Instead I think it would be best to automatically enable functions depending on what version of curl was found by curl-sys.

sagebind commented 2 years ago

I'm not sure that auto-discovery based on curl version is a good idea. If we dynamically link because static-curl isn't enabled and curl is available on the build machine, we might enable new features that the developer might use, not aware that they're not available on old versions. When you try to run the program on a system with an older curl version, suddenly things don't work because one or more symbols are missing.

I wouldn't assume that the curl version on the build machine is necessarily the version the developer wants to target. Ideally there'd be a way for the developer to specify a "target minimum curl version" and we'd automatically enable any additional functionality available on that version or higher.

Maybe we can get by with using version numbers as features? For example:

[features]
v7_62 = []
v7_68 = ["v7_62"]
    #[cfg(feature = "v7_62")]
    pub fn curl_easy_upkeep(curl: *mut CURL) -> CURLcode;

    #[cfg(feature = "v7_68")]
    pub fn curl_multi_poll(
        multi_handle: *mut CURLM,
        extra_fds: *mut curl_waitfd,
        extra_nfds: c_uint,
        timeout_ms: c_int,
        ret: *mut c_int,
    ) -> CURLMcode;

    #[cfg(feature = "v7_68")]
    pub fn curl_multi_wakeup(multi_handle: *mut CURLM) -> CURLMcode;

Basically by default, none of the version features are enabled and we only expose symbols that are available on relatively old curl versions. If you want to target/require a newer version, you can select a particular version feature (which automatically enables all older version features) and all newer symbols are available to you. Still a little bit of a chore, but more maintainable than a feature per function I think. This is the approach that the GTK crate takes, for example.

darkprokoba commented 2 years ago

@alexcrichton, if you're fine with @sagebind's proposal, i'll go ahead with it...

alexcrichton commented 2 years ago

I don't really know the best way to do this myself. I personally really don't like micro-managing versions all the time, I want things to "just work" where if there's an API I want to be able to use it. I also personally really don't like being in a situation where some 4-year-old change can't be used because one person is on some ancient OS and is blocking everyone else from upgrading some shared dependency.

I don't think the system you're talking about though @sagebind jives well with what we already have today. The majority of curl's new features go through options which means that it's always "available" you just get errors setting an unknown option if your version of curl is too old. In that sense we would need to apply this feature system to all of the option-setting methods as well, which seems pretty tedious.

The main alternative that I can think of for this is:

A scheme like this is more geared towards developers of the crate ("just add the API and make it an error on older versions"). Users of the crate would get an error when they used an API from a newer version of curl, didn't enable the version feature on this crate (which I expect will be common), and then link against some system version that's too old.

In any case I agree that a feature-per-function isn't necessary and a feature-per-version is the way to go. I'm mostly worried about the multiplicative approach of pushing enabling these features to all end-users. As time goes on it's less and less likely someone has a too-old version of curl to use the APIs in this crate, and that's sort of what I'd prefer to optimize for (the future users of the crate rather than the exact moment of time we're in right now)

sagebind commented 2 years ago

I don't think the system you're talking about though @sagebind jives well with what we already have today. The majority of curl's new features go through options which means that it's always "available" you just get errors setting an unknown option if your version of curl is too old. In that sense we would need to apply this feature system to all of the option-setting methods as well, which seems pretty tedious.

Most of curl's new features do go through options, and I think the current behavior of returning an error if not supported is perfectly fine and I think in the spirit of how curl operates, so I don't think we need to gate options by feature-versions (although if we were to adopt that it would make things more consistent overall). The problem is new function or struct definitions which will give you link-time errors or execution errors the developer can't recover from.

alexcrichton commented 2 years ago

Oh I'm not saying we should give link-time errors, what I'm saying is that we can detect the version of curl found in the curl-sys crate and feed that into the curl crate which would then conditionally define functions like wakeup as always-return-an-error or call-the-actual-function depending on whether it's available or not.

Basically I'm pointing out how using future options should probably have the same story as using future methods where possible.