-
Notifications
You must be signed in to change notification settings - Fork 237
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
Enable the upkeep_7_62_0 and poll_7_68_0 features in CI #414
base: main
Are you sure you want to change the base?
Conversation
…curl_static feature is enabled.
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 |
I'm not sure that auto-discovery based on curl version is a good idea. If we dynamically link because 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. |
@alexcrichton, if you're fine with @sagebind's proposal, i'll go ahead with it... |
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 |
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. |
Oh I'm not saying we should give link-time errors, what I'm saying is that we can detect the version of Basically I'm pointing out how using future options should probably have the same story as using future methods where possible. |
Enable the upkeep_7_62_0 and poll_7_68_0 features in CI whenever the curl_static feature is enabled.