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

Use environment variables to specify OpenSSL location on OS X #479

Closed
aneeshusa opened this issue Sep 14, 2016 · 5 comments
Closed

Use environment variables to specify OpenSSL location on OS X #479

aneeshusa opened this issue Sep 14, 2016 · 5 comments
Assignees

Comments

@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Sep 14, 2016

OpenSSL is a key-only homebrew crate, and doing brew link --force openssl means we

may end up linking against the insecure,
deprecated system OpenSSL while using the headers from Homebrew's openssl.

Instead, we need to explicitly pass the full include/lib paths to the compiler on our OS X builders, which is most easily done via environment variables.

Parts:

  • Update Salt code to no longer call brew link --force openssl
  • Update Buildbot configuration to pass correct values for the OPENSSL_INCLUDE_DIR and OPENSSL_LIB_DIR environment variables
  • Run some command (TODO: which one?) to unlink openssl on our OS X builders

@santagada Can you give me example output for "$(brew --prefix)"? Does this output vary (over time, with openssl package version, randomly, with homebrew updates)? How this varies will inform how we get the right environment variables, due to the property that the Buildbot configuration is executed on the Buildbot master (a Linux machine), not the actual builder. E.g., we may lock down the OpenSSL version and template the string, or we may dynamically get the version, or just wait for the move to NSS/ring/crypto-library-of-your-choice-here.

Companion to servo/servo#13225.

@santagada
Copy link

@santagada santagada commented Sep 14, 2016

I think it is pretty stable, for the general user docs I would leave the command there but for salt we can fix it to /usr/local/opt/openssl and just know that if in the future all builds on osx always fails on linking openssl is because of a more from homebrew.

@aneeshusa
Copy link
Member Author

@aneeshusa aneeshusa commented Sep 14, 2016

@santagada Due to how Buildbot works, it's much easier to set static environment variables than dynamic ones (i.e. those that shell out, which is the "$(brew --prefix openssl)" part). Is the output of that command always /usr/local/opt/openssl, or does it change based on the openssl version?

@aneeshusa
Copy link
Member Author

@aneeshusa aneeshusa commented Sep 14, 2016

OK, from IRC it sounds like the output of that command should always be /usr/local/opt/openssl, so we can hardcode the environment variable values for now. @santagada, you were interested in this so I'll assign this to you.

Files to change:

  • servo-build-dependencies/init.sls: Remove the salt state which runs brew link --force openssl
  • buildbot/master/files/config/environments.py: Add the new environment variables to the build_mac Environment.

Tests:

  • No tests needed (aside from getting a successful build).

We also need to somehow "unlink" openssl on our existing builders, so if you know a command for that we can run it by hand.

@aneeshusa
Copy link
Member Author

@aneeshusa aneeshusa commented Sep 24, 2016

@santagada I actually have taken care of this in #483, as part of upgrading Homebrew to 1.0.0. Feel free to work on another issue, and thanks again for your initial report and PR to make our OpenSSL usage via Homebrew better!

bors-servo added a commit that referenced this issue Sep 27, 2016
Fix Homebrew 1.0.0 fallout

Homebrew 1.0.0 has just been released with some breaking changes. To handle all of them, this PR has a lot of changes, here are the main points:
- Add a new `homebrew` execution module and `hombrew_analytics` state module to fix analytics disabling
- Fix `autoconf`/`autoconf213` installing and linking
- Use env vars to specify OpenSSL location because linking fails (fixes #479)
- Upgrade to XCode 8/OS X 10.11 on Travis (fixes #382) to fix installing git
- Update Salt, leaping two major versions ahead to 2016.3.3 (the latest), due to changes in Homebrew Python/Salt packaging not available in our pinned old Salt
- Make our install and dispatch scripts more robust:
  - Force apt-get to use existing configuration files and ignore updates in Salt packages during installation
  - Unlink Salt if we have installed it via Homebrew previously before installing a new one
  - Allow failures when building old revs when checking upgrades (upstreams change)
  - Manually invalidate the Salt cache between runs

See commit messages for full details.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/483)
<!-- Reviewable:end -->
@santagada
Copy link

@santagada santagada commented Sep 27, 2016

I'm on vacation, sorry for not unassining myself, and thanks for fixing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.