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

travis: add boringssl build #2118

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@bagder
Member

bagder commented Nov 27, 2017

No description provided.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 27, 2017

Member

@davidben! Do you have a suggested tag/tag system we should use when we run test builds of curl/boringssl, or is using borginssl's master branch reasonably safe?

Member

bagder commented Nov 27, 2017

@davidben! Do you have a suggested tag/tag system we should use when we run test builds of curl/boringssl, or is using borginssl's master branch reasonably safe?

Show outdated Hide outdated .travis.yml
@@ -108,6 +112,20 @@ before_script:
sudo make install
)
fi
if [ "$TRAVIS_OS_NAME" = linux -a "$BORINGSSL" ]; then
(git clone https://boringssl.googlesource.com/boringssl &&
cd boringssl &&

This comment has been minimized.

@wmark

wmark Nov 27, 2017

Contributor

Some revisions need this:

sed -i -e '/^if (BUILD_SHARED_LIBS)/i\add_definitions(-DBORINGSSL_ENABLE_DHE_TLS)' CMakeLists.txt &&
@wmark

wmark Nov 27, 2017

Contributor

Some revisions need this:

sed -i -e '/^if (BUILD_SHARED_LIBS)/i\add_definitions(-DBORINGSSL_ENABLE_DHE_TLS)' CMakeLists.txt &&
Show outdated Hide outdated .travis.yml
mkdir build &&
cd build &&
cmake -DCMAKE_INSTALL_PREFIX:PATH=$HOME/build-boringssl .. &&
make &&

This comment has been minimized.

@wmark

wmark Nov 27, 2017

Contributor

Using cmake with this option is probably what most maintainers use:

cmake -DCMAKE_BUILD_TYPE=release …

… and then you could decrease compilation time by (replace ninja by make; don't forget libdecrepit.a):

ninja crypto/libcrypto.a ssl/libssl.a decrepit/libdecrepit.a

FYI, you could build everything as shared libraries, but I don't think this makes a difference for the tests.

@wmark

wmark Nov 27, 2017

Contributor

Using cmake with this option is probably what most maintainers use:

cmake -DCMAKE_BUILD_TYPE=release …

… and then you could decrease compilation time by (replace ninja by make; don't forget libdecrepit.a):

ninja crypto/libcrypto.a ssl/libssl.a decrepit/libdecrepit.a

FYI, you could build everything as shared libraries, but I don't think this makes a difference for the tests.

This comment has been minimized.

@bagder

bagder Nov 27, 2017

Member

libdecrepit.a

What's that and what do we need that for?

@bagder

bagder Nov 27, 2017

Member

libdecrepit.a

What's that and what do we need that for?

This comment has been minimized.

@wmark

wmark Nov 27, 2017

Contributor

»Old« algorithms are collected in that library. For example, DSA and RC4.

@wmark

wmark Nov 27, 2017

Contributor

»Old« algorithms are collected in that library. For example, DSA and RC4.

This comment has been minimized.

@bagder

bagder Nov 27, 2017

Member

And regarding ninja: I dread to add more dependencies (problems) to the annoying travis environment. Besides, make vs ninja can't make that big difference when building from scratch in a single-core one-by-one build surely?

@bagder

bagder Nov 27, 2017

Member

And regarding ninja: I dread to add more dependencies (problems) to the annoying travis environment. Besides, make vs ninja can't make that big difference when building from scratch in a single-core one-by-one build surely?

This comment has been minimized.

@wmark

wmark Nov 27, 2017

Contributor

Never used make here, but I think make accepts what-to-build as arguments like ninja does.

(I guess you want to use make -j$(nproc) … here.)

@wmark

wmark Nov 27, 2017

Contributor

Never used make here, but I think make accepts what-to-build as arguments like ninja does.

(I guess you want to use make -j$(nproc) … here.)

This comment has been minimized.

@wmark

wmark Nov 27, 2017

Contributor

Tried it, make -j$(nproc) works, using make you cannot compile a subset of files like when using ninja.

@wmark

wmark Nov 27, 2017

Contributor

Tried it, make -j$(nproc) works, using make you cannot compile a subset of files like when using ninja.

This comment has been minimized.

@bagder

bagder Nov 27, 2017

Member

using make you cannot compile a subset of files like when using ninja.

so you're saying the generated makefile builds too many files?

@bagder

bagder Nov 27, 2017

Member

using make you cannot compile a subset of files like when using ninja.

so you're saying the generated makefile builds too many files?

This comment has been minimized.

@wmark

wmark Nov 27, 2017

Contributor

Yes, but there's no harm in that. Other than some cpu cycles.

@wmark

wmark Nov 27, 2017

Contributor

Yes, but there's no harm in that. Other than some cpu cycles.

This comment has been minimized.

@bagder

bagder Nov 27, 2017

Member

we can consider changing that in phase two, just now I want to focus on getting this to work...

@bagder

bagder Nov 27, 2017

Member

we can consider changing that in phase two, just now I want to focus on getting this to work...

Show outdated Hide outdated .travis.yml
@@ -108,6 +112,20 @@ before_script:
sudo make install
)
fi
if [ "$TRAVIS_OS_NAME" = linux -a "$BORINGSSL" ]; then
(git clone https://boringssl.googlesource.com/boringssl &&

This comment has been minimized.

@wmark

wmark Nov 27, 2017

Contributor
git clone --depth=1 …

… if you don't rewind to the latest tag or any blessed revision, using a shallow clone will be faster.

@wmark

wmark Nov 27, 2017

Contributor
git clone --depth=1 …

… if you don't rewind to the latest tag or any blessed revision, using a shallow clone will be faster.

This comment has been minimized.

@bagder

bagder Nov 27, 2017

Member

good idea!

@bagder

bagder Nov 27, 2017

Member

good idea!

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 28, 2017

Member

Hm, the build works now, but we get memory leakage from inside boringssl. I'll try to reproduce this locally and see what's going on.

Member

bagder commented Nov 28, 2017

Hm, the build works now, but we get memory leakage from inside boringssl. I'll try to reproduce this locally and see what's going on.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 29, 2017

Member

It seems to be a false positive on thread-local memory, possibly caused by a buggy/old valgrind version. Since we already run many tests with valgrind enabled, I figured switching it off for the boringssl build is an okay work-around.

Member

bagder commented Nov 29, 2017

It seems to be a false positive on thread-local memory, possibly caused by a buggy/old valgrind version. Since we already run many tests with valgrind enabled, I figured switching it off for the boringssl build is an okay work-around.

@wmark

This comment has been minimized.

Show comment
Hide comment
@wmark

wmark Nov 29, 2017

Contributor

Remains the issue of what revision to pick for tests.

In the ticket leading up to this one I suggested to not go with HEAD because sometimes breaking changes occur (which sometimes are reverted, sometimes not).

edit: Absent any better suggestion I'd like to repeat mine: Rewind to the latest tag for cocoapods, or any that matches version_for. On any substantial changes this could be revisited.

Contributor

wmark commented Nov 29, 2017

Remains the issue of what revision to pick for tests.

In the ticket leading up to this one I suggested to not go with HEAD because sometimes breaking changes occur (which sometimes are reverted, sometimes not).

edit: Absent any better suggestion I'd like to repeat mine: Rewind to the latest tag for cocoapods, or any that matches version_for. On any substantial changes this could be revisited.

fixup: use separate build without --enable-debug and no valgrind
The debug option causes far too many warnings in boringssl headers.
Valgrind triggers some false positive errors in thread-local data
used by boringssl.
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 29, 2017

Member

I asked David Benjamin and he said:

If you care about a "reasonably up-to-date" BoringSSL (which is also how we typically do things) just tracking master probably makes sense then.

... so I figure we can try doing that and see how it goes.

Member

bagder commented Nov 29, 2017

I asked David Benjamin and he said:

If you care about a "reasonably up-to-date" BoringSSL (which is also how we typically do things) just tracking master probably makes sense then.

... so I figure we can try doing that and see how it goes.

@wmark

This comment has been minimized.

Show comment
Hide comment
@wmark

wmark Nov 29, 2017

Contributor

Good enough for me. Thanks!

Contributor

wmark commented Nov 29, 2017

Good enough for me. Thanks!

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 30, 2017

Member

The build looks red here only because I cancelled all the non-boringssl builds. I'll proceed and merge this into master now...

Member

bagder commented Nov 30, 2017

The build looks red here only because I cancelled all the non-boringssl builds. I'll proceed and merge this into master now...

@bagder bagder closed this in 270494e Nov 30, 2017

@bagder bagder deleted the bagder/travis-boringssl branch Nov 30, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.