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

Fix pkgconfig for openssl build #1054

Closed
wants to merge 1 commit into from

Conversation

malbarbo
Copy link
Contributor

To avoid cache problems #1028 uses a temporary install directory for openssl. When the openssl install is completed the install dir is renamed. In doing so the pkgconfig files become invalid and libcurl-sys fails to find openssl and is built without https support. This fix the pkgconfig files. Fixes #1051.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 12, 2017

@malbarbo Thanks, this looks important to fix ASAP

However, modifying pkgconfig directly with a regex seems a bit brittle, and if it breaks, we won't realise until too late.

What do you think of the following:

  1. Cache the build directory instead of the install directory (the build directory can be safely renamed to retain the atomicity of the cache update, without breaking any paths) and run the make install command on every build (I would expect it to be relatively fast, but if that's not the case we can rethink).

  2. Add a test to verify that curl was built with ssl support, whatever's easiest to implement, just so long as a regression here causes the build to fail.

@malbarbo malbarbo force-pushed the fix-openssl-build branch 3 times, most recently from d848917 to 1b24154 Compare April 12, 2017 00:57
@malbarbo
Copy link
Contributor Author

@Diggsey I added a test to verify that rustup-init was built with ssl support.

I don't known exactly how to do the first point and I'm afraid that I cannot investigate it right now. Feel free to continue this fix.

Before a new release, consider fixes for #1047, #1055 and #1058

@malbarbo
Copy link
Contributor Author

Docker was using the cached openssl build... Let's see how it goes now.

@malbarbo
Copy link
Contributor Author

@Diggsey travis is green now.

@malbarbo
Copy link
Contributor Author

@brson Considering that this is an important fix, you may be interest.

@jelford
Copy link
Contributor

jelford commented Apr 13, 2017

Fwiw a revert of #1028 would also be a reasonable option as a quick fix (it's strictly an optimization - when it works - so the only hit would be build time).

jelford added a commit to jelford/rustup.rs that referenced this pull request Apr 14, 2017
@malbarbo
Copy link
Contributor Author

Closing in favor of #1065;

@malbarbo malbarbo closed this Apr 14, 2017
bors added a commit that referenced this pull request Apr 15, 2017
Fix OpenSSL linkage by using the final install-directory in the build

This PR addresses #1051 by avoiding moving OpenSSL's install-target directory after it's been configured. It also encorporates the regression test suggested by @malbarbo on #1054.

It still makes sense to move directories about to avoid getting a partially-built copy of openssl, and I think it makes sense to cache the finished product rather than the src/build directory.

I haven't been able to test the output on one of the affected platforms (although the check on symbols passes) as I don't know a good way to get artefacts out of the travis build.
nodakai pushed a commit to nodakai/rustup.rs that referenced this pull request Apr 23, 2017
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.

rustup update fails and erroneously mention no https support in curl
3 participants