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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

wget 1.19.2 build script #521

Merged
merged 5 commits into from
Dec 13, 2017
Merged

wget 1.19.2 build script #521

merged 5 commits into from
Dec 13, 2017

Conversation

ingalls
Copy link
Contributor

@ingalls ingalls commented Nov 27, 2017

First pass at writing a build script for mason. Attempting to add wget@1.19.2

@springmeyer How far off the mark am I? Would love 馃憗

@springmeyer
Copy link
Contributor

@ingalls - happy to give 馃憗 .

Overall this script is looking good. The main missing piece is that wget has dependencies that also need to be packaged. The idea behind also packaging the dependencies in mason is that allows the final wget binary to statically link those dependencies and therefore not have any runtime dependencies on any libraries from the system that might complicate portability.

So it looks like you'll also need to package gnutls. But there is gocha: libraries dealing with SSL/HTTP need to have access to a set of SSL certificates. And these are usually managed by the system installation of openssl. So, my sense is that we'll want to dynamically link wget to openssl (also a good idea to leverage the system openssl security updates). Then the questions become:

  • Does gnutls depend on openssl, and if so, can it depend on the system openssl?
  • What other deps does wget have?


PREFIX=${MASON_PREFIX} \
CXXFLAGS="${CXXFLAGS}" \
LDFLAGS="${LDFLAGS} -ldl -lpthread" make install
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 23 -> 29 can likely be removed. The reason is that build systems that use ./configure usually pick up these environment variables automatically, such that they do not need to be sent directly to make. A good example to follow for a ./configure based build is harfbuzz: https://github.com/mapbox/mason/blob/master/scripts/harfbuzz/1.4.4-ft/script.sh#L36-L61. In harfbuzz the build variables are setup before ./configure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

馃憤 Removed & Tested


function mason_load_source {
mason_download \
http://ftp.gnu.org/gnu/wget/wget-1.19.2.tar.gz \
Copy link
Contributor

Choose a reason for hiding this comment

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

Best practice is to do http://ftp.gnu.org/gnu/wget/${MASON_NAME}-${MASON_VERSION}.tar.gz such that creating new package versions, based on cp -r <this package> <new package, is 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.

馃憤 Fixed

Copy link
Contributor

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

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

Need to install gnutls and other other deps in the mason_prepare_compile hook

@ingalls
Copy link
Contributor Author

ingalls commented Nov 27, 2017

@springmeyer 馃憤 As far as I can tell GnuTLS is the only requested library that needs to be installed but I'll go through the wget configuration and double check that this is the case.

Reading the docs it looks like gnutls is separate from openssl and does not require it. There is a flag to use openssl instead of gnutls in wget if we want to go that route. For now I've sketched out a script for compiling gnu tls and will see if I can get that up and running

@springmeyer
Copy link
Contributor

As far as I can tell GnuTLS is the only requested library that needs to be installed but I'll go through the wget configuration and double check that this is the case.

Interesting. Does GnuTLS have deps itself? A quick way to check would be to do brew edit gnutls or try building locally and see what happens ./mason build gnutls 3.5.16.

Reading the docs it looks like gnutls is separate from openssl and does not require it.

Oh, interesting.

There is a flag to use openssl instead of gnutls in wget if we want to go that route.

Okay. I presume then that means either gnutls or openssl will be the library that knows where certs are.

If we depend on gnutls we'd need to statically link it and provide our own certs. This could get messy.

If we depend on openssl we can likely expect linux/osx systems to have a recent enough system openssl to use that (and not have to bundle certs ourself).

For now I've sketched out a script for compiling gnu tls and will see if I can get that up and running

Based on the above cert issue, it feels like trying to use system openssl will be an easier route. That said, if gnutls+bundling our own certs is likely feasible, just could be a lot of work/unpredictable amount of time to get it right.

@ingalls ingalls dismissed springmeyer鈥檚 stale review November 28, 2017 14:59

Requesting a new review :)

@ingalls
Copy link
Contributor Author

ingalls commented Nov 28, 2017

@springmeyer
Copy link
Contributor

@ingalls looks like the OS X build is failing with configure: error: openssl development library not found for MD5: https://travis-ci.org/mapbox/mason/jobs/308527481#L530.

It appears that the problem is that while /usr/lib/libcrypto.dylib is available, apple has not included it in the SDK at /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk which is used as the sysroot. Because /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/lib/libcrypto.dylib does not exist, the linker refuses to find it. The last comment at https://forums.developer.apple.com/thread/52082 indicates that apple decided to restrict this and instead force devs to statically link libcrypto. This is okay, we can do that, it just means to me that we'll need to statically link AND still find a way to point at the system certificates (to avoid bundling). I don't know how to do this - would take more research time that I currently have. Ideas:

  • a) merge with OS X knowingly broken and ticket getting it working
  • b) invest time in trying to get libcrypto working on OS X

I'm open to either, but I don't have the ability to invest in b right now.

@ingalls
Copy link
Contributor Author

ingalls commented Nov 30, 2017

@springmeyer I have no access to a Mac OSX Build environment to play around with this so if I did so would have to get setup with another laptop.

So I'd suggest we either merge with it broken and ticket or hold on this ticket until we can have some time to be in the same place? I'm of course partial to (a) as I run mason on my local linux dev machine as well as all our ubuntu docker images.

@springmeyer
Copy link
Contributor

@ingalls let's merge. Can you please ticket the OS X issue?

Copy link
Contributor

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

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

馃憤

@springmeyer
Copy link
Contributor

@ingalls next action is in your court - please merge and ticket OS X (the fact that it does not work yet) when you can.

@ingalls ingalls merged commit a810d05 into master Dec 13, 2017
@ingalls ingalls deleted the wget branch December 13, 2017 18:28
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.

None yet

2 participants