Exposed the curl_multi_poll libcurl API behind a new feature flag: po…

[darkprokoba]

…ll_7_66_0, see #403.

See also: https://curl.se/libcurl/c/curl_multi_poll.html

[alexcrichton]

Looks good to me! I think the only other thing is dealing with poll_7_66_0. If you'd like we can leave it, but I also think it's fine to remove it entirely. If it's used with a build of curl that doesn't have the function you'd get a link error, but that's somewhat reasonable to me so long as the documentation mentions it. Otherwise if it's not used you can still use this library with older versions.

[darkprokoba]

In this case, I think we'd better off without it :-)

[darkprokoba]

I got rid of the feature.

that's somewhat reasonable to me so long as the documentation mentions it.

Where's the right place to document this? I've already mentioned "Requires libcurl 7.66.0 or later." in the rustdoc for the Multi::poll method?

[darkprokoba]

`curl::multi::Multi::poll::h240210e8eebc7cfc':
          /src/src/multi.rs:654: undefined reference to `curl_multi_poll'
          collect2: error: ld returned 1 exit status

hhm, maybe we do need the feature after all?

[alexcrichton]

Hm ok well that can be solved but it's fine to leave the feature in. Could the static-curl feature in the manifest be changed to automatically activate this feature? I fear that this will almost never be otherwise activated, but we can at least guarantee its presence with the static-curl feature enabled since we know the version of curl used will have the function. (this can also be done for the upkeep_7_62_0 I just didn't think of this when that was added)

[darkprokoba]

I actually don't have any preference on whether we should leave the feature in or not. I definitely lack the knowledge to make that call. I'd appreciate your perspective on that?

Hm ok well that can be solved

Great, though I'm afraid I've not the faintest idea how :-)

Could the static-curl feature in the manifest be changed to automatically activate this feature?

do you mean I should change ./Cargo.toml:47 to look something like

static-curl = ["curl-sys/static-curl", "curl-sys/poll_7_68_0", "curl-sys/upkeep_7_62_0"]

[alexcrichton]

The reason that this failed on CI is that the -Clink-dead-code argument is forced for other unrelated reasons. We could fix CI by removing that test with the system version, and only passing the argument with the test against the built-in version.

Otherwise for the feature I think it would want to enable the corresponding feature on the curl crate itself to have the function get exposed.

[darkprokoba]

Alright, this -Clink-dead-code thing is way over my head, and anyways if we are to go ahead with this, perhaps we should do so in a separate changeset?

enable the corresponding feature on the curl crate itself

Sorry if I'm being a little thick here, but you don't mean we should enable the feature by default for everyone in the curl crate, right? Did you mean we should just enable it in some of the ci tests? Something like:

diff --git a/ci/run.sh b/ci/run.sh
index 5555422..a3f59cc 100755
--- a/ci/run.sh
+++ b/ci/run.sh
@@ -6,7 +6,7 @@ cargo test --target $TARGET --no-run
 # First test with no extra protocols enabled.
 cargo test --target $TARGET --no-run --features static-curl
 # Then with all extra protocols enabled.
-cargo test --target $TARGET --no-run --features static-curl,protocol-ftp
+cargo test --target $TARGET --no-run --features static-curl,protocol-ftp,poll_7_68_0
 if [ -z "$NO_RUN" ]; then
     cargo test --target $TARGET
     cargo test --target $TARGET --features static-curl
@@ -17,7 +17,7 @@ if [ -z "$NO_RUN" ]; then
     RUSTFLAGS=-Clink-dead-code \
     cargo run --manifest-path systest/Cargo.toml --target $TARGET
     RUSTFLAGS=-Clink-dead-code \
-    cargo run --manifest-path systest/Cargo.toml --target $TARGET --features curl-sys/static-curl,curl-sys/protocol-ftp
+    cargo run --manifest-path systest/Cargo.toml --target $TARGET --features curl-sys/static-curl,curl-sys/protocol-ftp,poll_7_68_0

     cargo doc --no-deps --target $TARGET
     cargo doc --no-deps -p curl-sys --target $TARGET

?

[alexcrichton]

Nah yeah this is fine to merge, a better feature story can happen later.