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

Support ALPN only if the underlying macOS supports ALPN methods #68

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@kzys

kzys commented Jul 25, 2018

No description provided.

@kzys

This comment has been minimized.

kzys commented Jul 25, 2018

Not so sure why 10.13 is failing. Maybe http://www.openradar.me/34790589 ?

I probably need to skip the ALPN-related tests when the symbol is missing, but I'd like to discuss about the direction of this change first.

@kornelski

This comment has been minimized.

kornelski commented Jul 25, 2018

Wouldn't it be better to use weak linking instead of dlopen?

@kzys

This comment has been minimized.

kzys commented Jul 25, 2018

Sadly weak linkage is nightly only, iirc.
rust-lang/rust#29603

Another option is targeting newer versions of macOS by default, like https://github.com/servo/core-foundation-rs

To enable features added in macOS 10.8, set Cargo feature mac_os_10_8_features. To have both 10.8 features and 10.7 compatibility, also set mac_os_10_7_support. Setting both requires weak linkage, which is is a nighty-only feature as of Rust 1.19.

@kornelski

This comment has been minimized.

kornelski commented Jul 25, 2018

Is ALPN available only for macOS 10.13+? If so, that's a bit high to aim for. I think currently 10.8-10.9 is the reasonable baseline.

So maybe ALPN itself could be a Cargo feature? It could be auto enabled when 10.13+ is set as minimum, and opt-in (needing weak/nightly) otherwise.

@sfackler

This comment has been minimized.

Owner

sfackler commented Jul 25, 2018

If we're going to move to a dlopen-based approach for functionality added in newer OS versions, I think we'll want to do it for the entire library rather than as a one-off for ALPN.

@sfackler

This comment has been minimized.

Owner

sfackler commented Jul 25, 2018

@kornelski ALPN support is currently only enabled if you turn on the OSX_10_13 Cargo feature - this PR is trying to change that to be detected at runtime.

@kornelski

This comment has been minimized.

kornelski commented Jul 25, 2018

I'm suggesting

[features]
alpn = []
OSX_10_13 = ["OSX_10_12", "alpn"]

and if both OSX_10_13 and alpn are set, use it unconditionally. If alpn is set, but not OSX_10_13, then use it via weak linking.

@kzys righly noted such approach is used by core foundation already: https://github.com/servo/core-foundation-rs/search?q=weak&unscoped_q=weak

@kzys kzys force-pushed the kzys:alpn branch 2 times, most recently from 941c366 to 9857315 Jul 26, 2018

@kzys kzys force-pushed the kzys:alpn branch from 9857315 to 06fc4a5 Jul 26, 2018

@kzys

This comment has been minimized.

kzys commented Jul 26, 2018

Works me. I've updated this pull request. Both "cargo test --features OSX_10_13" and "cargo test --features alpn" are working on my MacBook (macOS 10.13.6), while I haven't updated .circleci/config.yml yet.

@kzys

This comment has been minimized.

kzys commented Sep 11, 2018

@sfackler Could you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment