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

Statically link openssl for reqwest #6816

Merged

Conversation

illicitonion
Copy link
Contributor

Avoids platform-specific library location issues.

@cosmicexplorer
Copy link
Contributor

illicitonion/reqwest@983409e for anyone curious

@wisechengyi
Copy link
Contributor

What does the change mean?

native-tls-vendored = ["native-tls/vendored"]

@illicitonion
Copy link
Contributor Author

What does the change mean?

native-tls-vendored = ["native-tls/vendored"]

Crates can have optional features (in the [features] section of their Cargo.toml), which may both entail optional dependencies, and optional pieces of code: https://doc.rust-lang.org/cargo/reference/manifest.html#the-features-section

Our dependency chain here goes engine -> reqwest -> native-tls.

native-tls exposes a feature called vendored which will statically link openssl.
We want to enable that.
But you can't enable features on transitive deps, only on direct deps.
So my change to reqwest adds a feature to reqwest called native-tls-vendored which enables the vendored feature on its native-tls dependency.
Then my change here enables the native-tls-vendored feature on our reqwest dependency, which will in turn enable the vendored feature of its native-tls dependency, leading to openssl being statically linked :)

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@stuhood stuhood added this to the 1.12.x milestone Nov 27, 2018
@stuhood
Copy link
Member

stuhood commented Nov 28, 2018

Although the travis failure seems to be deterministic (https://travis-ci.org/pantsbuild/pants/jobs/460355339), I don't repro it locally. Will rebase this to master and give it another go.

@stuhood stuhood force-pushed the dwagnerhall/reqwest/static-link-openssl branch from eba55b2 to 80fd564 Compare November 28, 2018 00:25
@illicitonion illicitonion force-pushed the dwagnerhall/reqwest/static-link-openssl branch from 80fd564 to 2e3bb68 Compare November 28, 2018 08:22
@stuhood
Copy link
Member

stuhood commented Nov 28, 2018

@stuhood
Copy link
Member

stuhood commented Nov 28, 2018

Pushed a commit to "try harder" with regard to ulimit setting on that shard.

@wisechengyi
Copy link
Contributor

wisechengyi commented Nov 29, 2018

Breaking out the targets seems to do the trick.

@cosmicexplorer would you mind consolidating python_native_code_testing_(1-3) into more 1:1:1 compliant in a follow patch? so there would be no need for things like

python_tests(
  sources=globs('test_*.py', exclude=([globs('*_integration.py')] + python_native_code_test_files)),

Also I did not spend the time trimming unused dependencies for python_native_code_testing_(1-3)

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.

4 participants