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

add homebrew locations for openssl, icu4c, python3 #894

Merged
merged 1 commit into from
Feb 27, 2015

Conversation

Kriechi
Copy link
Contributor

@Kriechi Kriechi commented Feb 26, 2015

improves building on Mac OS X:

  • icu4c is auto-detected if installed via Homebrew
  • openssl is auto-detected if installed via Homebrew
  • python3 is auto-detected if installed via Homebrew

The default is now to use openssl from Homebrew if available, as fallback it uses the system openssl.
If the environment variable USE_SYSTEM_OPENSSL is "true" then the system openssl is always used, no matter if a Homebrew version is installed.

Travis now tests using the Homebrew OpenSSL and the system OpenSSL on OS X.

@Mikaela
Copy link
Contributor

Mikaela commented Feb 26, 2015

How about LibreSSL? :)

I think it's also in homebrew.

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 26, 2015

@Mikaela since ZNC was not yet tested against LibreSSL so I didn't think about it.
Not sure if @DarthGandalf wants to support other TLS implementations (openssl-compatible)?

@espadolini
Copy link
Contributor

libressl works, but it needs an extra header to be included in csocket or it won't compile

@Mikaela
Copy link
Contributor

Mikaela commented Feb 26, 2015

it needs an extra header to be included in csocket or it won't compile

What kind of header? Does it break OopenSSL or anything else? If not, it should probably be PRed to Csocket.

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 26, 2015

Additional headers could be added via configure as well.
If the ZNC maintainers officially want to support libressl I'm happy to work on a second PR.
Either way - out of the scope of this PR.

@Mikaela
Copy link
Contributor

Mikaela commented Feb 26, 2015

I opened jimloco/Csocket#47

@Mikaela
Copy link
Contributor

Mikaela commented Feb 26, 2015

On .configure, I think you should use $(command) instead of command as that is what seems to be done on other parts of that file.

See also:

@Kriechi Kriechi force-pushed the improve-osx-build branch 2 times, most recently from 5121c97 to 18bd05e Compare February 26, 2015 14:34
@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 26, 2015

@Mikaela thanks - I changed it accordingly.

@DarthGandalf
Copy link
Member

Proper openssl (like one from homebrew) is already tested in travis as part of linux test.
But osx has a different openssl itself, so I made travis to test that broken openssl instead of homebrew's one.

@@ -50,10 +50,8 @@ install:
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew config; fi
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew list --versions; fi
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew update; fi
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew install swig python3 icu4c jq; fi
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew install jq swig python3 icu4c openssl; fi
Copy link
Member

Choose a reason for hiding this comment

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

After this change jq is not needed anymore

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 was not sure - because in the next line you use it print all installed versions from brew...
Not sure if it any benefit...

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 26, 2015

ok - so not installing openssl via homebrew is ok for you?
or do you always want to configure with the OSX openssl?

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 26, 2015

https://github.com/Homebrew/homebrew/blob/master/Library/Formula/znc.rb#L26

so the Homebrew ZNC formula currently depends on openssl (from homebrew), not sure how that is working...

@DarthGandalf
Copy link
Member

You can add another travis build, with different configurations, to test more (libressl/openssl from homebrew/etc). But I'm not sure it's very different from what happens on linux.
Stock openssl of OSX needs to be tested, somehow.
As for formula, I believe, it autoadds those paths itself while building ZNC from inside brew.

@Mikaela
Copy link
Contributor

Mikaela commented Feb 26, 2015

As for formula, I believe, it autoadds those paths itself while building ZNC from inside brew.

Someone (@Kriechi?) was on #znc earlier today asking how to build ZNC using brewed OpenSSL and I also think that it's better to install from there than possibly very old OS X's version of OpenSSL. I also got the picture that they wanted to compile ZNC by themselves possibly with custom modules which means compiling outside of Homebrew.

@Kriechi Kriechi force-pushed the improve-osx-build branch 5 times, most recently from 402ef4c to a0d386f Compare February 26, 2015 16:59
@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 26, 2015

I changed the Travis build matrix to do two jobs on OS X:

  • default openssl
  • openssl via Homebrew

I checked the Homebrew formula for ZNC, and yes it does automatically add homebrew-openssl paths.

As @Mikaela pointed out, this PR is of use if somebody wants to compile ZNC from the repo or their own fork. It is better to include the homebrew openssl because the OSX version is deprecated and might get removed with the next OSX release. And it creates lots of nasty warnings during compilation.

@Kriechi Kriechi force-pushed the improve-osx-build branch 3 times, most recently from d93d61b to f88701b Compare February 26, 2015 21:14
@DarthGandalf
Copy link
Member

@Mikaela $() doesn't work in sh. `` does.

python3_prefix=$(dirname "$(find "$($BREW --prefix python3)/" -name python3.pc)")
export PKG_CONFIG_PATH="$python3_prefix:$PKG_CONFIG_PATH"

if test -z "$USE_SYSTEM_OPENSSL" -o "$USE_SYSTEM_OPENSSL" = "false"; then
Copy link
Member

Choose a reason for hiding this comment

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

I don't think configure should depend on travis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO this does not depend on travis.
If the user want to compile ZNC using the system openssl USE_SYSTEM_OPENSSL can be used in the same way.
Maybe I should add a description line to the configure script so this is documented somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Ok. But please add some ifs for ssl/python/icu: if they are installed in homebrew, put some message that using version from homebrew.
Makes debugging easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to do that. I only add the pkg-config path here. If PKG_CHECK_MODULES finds it in there is a different question...
Maybe we should just print a summary of found libs at the end (with some nicer formatting)?

echo $openssl_LIBS
echo $python_LIBS
echo $icu_LIBS

@Mikaela
Copy link
Contributor

Mikaela commented Feb 27, 2015

@Mikaela $() doesn't work in sh. `` does.

mikaela@terasar ~
% sh
$ echo Hi $(whoami)
Hi mikaela

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 27, 2015

@DarthGandalf @Mikaela yes sh should support $(...).

% ls -lah /bin/sh
-r-xr-xr-x  1 root  wheel   1.2M Jan 31 12:57 /bin/sh
% /bin/sh
sh-3.2$ echo Hi $(date)
Hi Fri Feb 27 10:21:28 GMT 2015

(tested on OSX with a real sh)

% ls -lah /bin/sh
lrwxrwxrwx 1 root root 4 Mar  1  2012 /bin/sh -> dash
% /bin/sh
$ echo Hi $(date)
Hi Fri Feb 27 11:06:38 CET 2015

(tested on Debian 7.8 with dash)

see http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html#tag_23_02_06_03
see http://unix.stackexchange.com/a/126928 (third excerpt box)

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 27, 2015

I added a description for the USE_SYSTEM_OPENSSL variable on OSX.

$ ./configure -h
[...]
Some influential environment variables:
[...]
  USE_SYSTEM_OPENSSL
              set USE_SYSTEM_OPENSSL="true" to use the OpenSSL version
              provided by Mac OS X. Otherwise the Homebrew installed version
              is used if available.

@Kriechi Kriechi changed the title add homebrew locations for openssl and icu4c add homebrew locations for openssl, icu4c, python3 Feb 27, 2015
@DarthGandalf
Copy link
Member

Well, dash is not real sh.
If sh is just an alias for bash/dash, of course bash/dash understands $().
Plain sh does not, however.

2015-02-27 2:38 GMT-08:00 Thomas Kriechbaumer notifications@github.com:

I added a description for the USE_SYSTEM_OPENSSL variable on OSX.

$ ./configure -h
[...]
Some influential environment variables:
[...]
USE_SYSTEM_OPENSSL
set USE_SYSTEM_OPENSSL="true" to use the OpenSSL version
provided by Mac OS X. Otherwise the Homebrew installed version
is used if available.


Reply to this email directly or view it on GitHub
#894 (comment).

@DarthGandalf
Copy link
Member

AFAIR, some BSD had a real sh

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 27, 2015

@DarthGandalf still not convinced, configure.ac already contains $(...)
https://github.com/znc/znc/blob/master/configure.ac#L166

would you like me to change it?

@Kriechi
Copy link
Contributor Author

Kriechi commented Feb 27, 2015

I changed the $(...) back to use backticks.
I added a short summary at the end of the configure output to indicate the found & used library paths.

@DarthGandalf
Copy link
Member

Oh, that was overlooked maybe. We just don't have enough tests for such broken systems.

DarthGandalf added a commit that referenced this pull request Feb 27, 2015
add homebrew locations for openssl, icu4c, python3
@DarthGandalf DarthGandalf merged commit a1165cf into znc:master Feb 27, 2015
@Kriechi Kriechi deleted the improve-osx-build branch February 27, 2015 16:55
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