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

Simplify build on macOS #35570

Merged
merged 14 commits into from
May 7, 2020
Merged

Simplify build on macOS #35570

merged 14 commits into from
May 7, 2020

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Apr 28, 2020

  • Check for OpenSSL (and pkg-config) in the check_prereqs() function
  • Simplify OpenSSL installation instructions
  • Automatically pick the OpenSSL version installed by Homebrew by exporting the proper PKG_CONFIG_PATH environment variable
  • Improve the error message if cmake can't find OpenSSL

The previous instructions were asking the user to add symbolic links inside /usr/local/lib/ and /usr/local/lib/pkgconfig/ for OpenSSL related files. There is no need for a complicated setup with symbolic links when the PKG_CONFIG_PATH environment variable points to the right pkgconfig, which is now done in build-commons.sh.

@ghost
Copy link

ghost commented Apr 28, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

* Check for OpenSSL (and pkg-config) in the check_prereqs() function
* Simplify OpenSSL installation instructions
* Automatically pick the OpenSSL version installed by Homebrew by exporting the proper PKG_CONFIG_PATH environment variable
* Improve the error message if cmake can't find OpenSSL

The previous instructions were asking the user to add symbolic links inside  /usr/local/lib/ and /usr/local/lib/pkgconfig/ for OpenSSL related files. There is no need for a complicated setup with symbolic links when the PKG_CONFIG_PATH environment variable points to the right pkgconfig, which is now done in build-commons.sh.
It's not needed since everything is properly handled in the System.Globalization.Native CMakeLists.txt file: https://github.com/dotnet/runtime/blob/e301ec16723437561c84f7b9a8c773def67b81fd/src/libraries/Native/Unix/System.Globalization.Native/CMakeLists.txt#L17

Anyway, recent versions of Homebrew refuse to link icu4c:

```sh
$ brew link --force icu4c
Warning: Refusing to link macOS provided/shadowed software: icu4c
If you need to have icu4c first in your PATH run:
  echo 'export PATH="/usr/local/opt/icu4c/bin:$PATH"' >> ~/.zshrc
  echo 'export PATH="/usr/local/opt/icu4c/sbin:$PATH"' >> ~/.zshrc

For compilers to find icu4c you may need to set:
  export LDFLAGS="-L/usr/local/opt/icu4c/lib"
  export CPPFLAGS="-I/usr/local/opt/icu4c/include"

For pkg-config to find icu4c you may need to set:
  export PKG_CONFIG_PATH="/usr/local/opt/icu4c/lib/pkgconfig"
```
eng/native/build-commons.sh Outdated Show resolved Hide resolved
Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

This was bugging me for a long time, thanks for fixing it!

eng/pipelines/common/global-build-job.yml Outdated Show resolved Hide resolved
eng/pipelines/libraries/build-job.yml Outdated Show resolved Hide resolved
Use install-native-dependencies.sh instead of duplicating the list of native dependencies in the build job yaml files, just like it's done in other build jobs (eng/pipelines/common/templates/runtimes/build-test-job.yml, eng/pipelines/coreclr/templates/build-job.yml and eng/pipelines/mono/templates/build-job.yml)
@akoeplinger
Copy link
Member

Looks like this gets further now but runtime-live-build and runtime (Installer Build and Test coreclr OSX_x64 Debug) seem to not run the install-native-dependencies.sh script so they fail right now.

@0xced
Copy link
Contributor Author

0xced commented Apr 28, 2020

I tried to fix the runtime pipeline wit 8f3eeb5 but I'm not sure if it's the best way.

The runtime-live-build pipeline fails with this error and I have no idea why and no idea how to diagnose this. 😕

/Users/runner/runners/2.166.4/work/1/s/eng/install-native-dependencies.sh: No such file or directory

@akoeplinger akoeplinger requested a review from a team April 28, 2020 22:25
@safern
Copy link
Member

safern commented Apr 28, 2020

FYI: @bartonjs @wfurt

Copy link
Contributor

@jashook jashook left a comment

Choose a reason for hiding this comment

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

Minus the open question about openssl 1.1 lgtm

ln -s /usr/local/opt/openssl\@1.1/lib/pkgconfig/libcrypto.pc /usr/local/lib/pkgconfig/
ln -s /usr/local/opt/openssl\@1.1/lib/pkgconfig/libssl.pc /usr/local/lib/pkgconfig/
ln -s /usr/local/opt/openssl\@1.1/lib/pkgconfig/openssl.pc /usr/local/lib/pkgconfig/
brew install cmake autoconf automake icu4c libtool openssl pkg-config python3
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if one can run tests locally without the links? (we would not see it the in CI as tests do run on different machine)

Copy link
Member

Choose a reason for hiding this comment

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

Assuming Homebrew is still not linking the libraries into /usr/local/lib/, we need to keep the runtime dependencies section.

But as long as src\libraries\Native\build-native.sh is sourcing the script that adds the default homebrew path to the pkg-config probe path, we can remove the pkgconfig-specific steps from the docs.

Copy link
Member

Choose a reason for hiding this comment

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

If this is meant for developers, I'm wondering if we can add rpath so runtime would find them as well automatically. Or add note that somebody can set LD_LIBRARY_PATH as alternative to linking.
In either case I would be nice if one does not have to do any linking and direct file manipulation.

@wfurt
Copy link
Member

wfurt commented Apr 28, 2020

Since you touching this, can you accommodate #34150 ? Perhaps have brew as the primary option.

if ! pkg-config openssl ; then
# We export the proper PKG_CONFIG_PATH where openssl was installed by Homebrew
# It's important to _export_ it since build-commons.sh is sourced by other scripts such as build-native.sh
export PKG_CONFIG_PATH=/usr/local/opt/openssl/lib/pkgconfig
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this should set the value additively, not as a replacement...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think replacement is correct here. build-commons.sh is sourced by other scripts and we really just need openssl. Also, the default directory will be searched anyway, see the pkg-config documentation about the PKG_CONFIG_PATH environment variable:

PKG_CONFIG_PATH
A colon-separated (on Windows, semicolon-separated) list of
directories to search for .pc files. The default directory will
always be searched after searching the path; the default is
libdir/pkgconfig:datadir/pkgconfig where libdir is the libdir
for pkg-config and datadir is the datadir for pkg-config when it
was installed.

@0xced
Copy link
Contributor Author

0xced commented Apr 28, 2020

Since you touching this, can you accommodate #34150 ? Perhaps have brew as the primary option.

Addressed in b2530b1. I removed the manual installation instructions altogether, the ICU installation a few lines below only has instruction with Homebrew anyway.

…ies.sh"

This reverts commit 277c5b5.

It turns out the sh was necessary, else we get this error when running on continuous integration:
> /Users/runner/runners/2.166.4/work/_temp/56691a53-a761-472c-b9ab-a9b4eb004f5d.sh: line 1: /Users/runner/runners/2.166.4/work/1/s/eng/install-native-dependencies.sh: Permission denied
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM. thanks.

@0xced
Copy link
Contributor Author

0xced commented May 6, 2020

Anything holding the merge?

@akoeplinger akoeplinger merged commit f6aad63 into dotnet:master May 7, 2020
@0xced 0xced deleted the simplify-macos-build branch May 7, 2020 09:13
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants