Update openssl location for osx homebrew#13225
Conversation
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon. |
highfive
added
the
S-awaiting-review
|
this is now complete and I can run servo without symlinks or any other changes. |
README.md
Outdated
| openssl/ include files, you may want to try: | ||
| ```sh | ||
| export DEP_OPENSSL_INCLUDE=/usr/local/include | ||
| export DEP_OPENSSL_INCLUDE=`brew --prefix openssl`/include |
There was a problem hiding this comment.
Please use
DEP_OPENSSL_INCLUDE="$(brew --prefix openssl)/include"
instead of backticks, per the style guide.
|
thanks @aneeshusa, done |
|
This looks good to me! Please squash and we can r+. |
|
@santagada Does this happen always or are there specific conditions that require this change? I'm wondering if we need to update our builder configuration. |
|
@aneeshusa according to both rust-openssl/rust-openssl#453 and my understanding of where brew puts its version of openssl I think it is necessary for the osx builders to do the same. |
|
@aneeshusa this seems wrong (as it doesn't work anymore) https://github.com/servo/saltfs/blob/master/servo-build-dependencies/init.sls#L66 changing the environment file (using the values here) and installing openssl from homebrew is the new way of doing it, and seems cleaner as well to not interfere with the openssl from osx. |
using the OPENSSL_* variables makes rust-openssl find the hombrew library without the need to link it to a common lib directory
|
@larsbergstrom squashed |
|
@santagada Does this require a version bump/minimum version of openssl from homebrew? Would you be willing to make a PR to saltfs? |
|
@bors-servo r+ |
|
📌 Commit a530032 has been approved by |
highfive
added
S-awaiting-merge
Update openssl location for osx homebrew <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [x] These changes do not require tests because its a documentation change <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> the location of openssl might have changed, but it reads as of now: ```sh $ brew link --force openssl Warning: Refusing to link: openssl Linking keg-only openssl means you may end up linking against the insecure, deprecated system OpenSSL while using the headers from Homebrew's openssl. Instead, pass the full include/library paths to your compiler e.g.: -I/usr/local/opt/openssl/include -L/usr/local/opt/openssl/lib ``` <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13225) <!-- Reviewable:end -->
|
💔 Test failed - linux-rel |
highfive
added
S-tests-failed
|
|
⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev are reusable. Rebuilding only linux-rel... |
|
@aneeshusa I'm willing to help on salt but have no idea were the code for osx are, can you point them to me (I can make the changes and do a PR) |
|
☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev |
6 participants
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsthe location of openssl might have changed, but it reads as of now:
This change is