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

travis: add boringssl build #2118

Closed
wants to merge 2 commits into from
Closed

travis: add boringssl build #2118

wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Nov 27, 2017

No description provided.

@bagder
Copy link
Member Author

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?

.travis.yml Outdated
@@ -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 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Some revisions need this:

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

.travis.yml Outdated
mkdir build &&
cd build &&
cmake -DCMAKE_INSTALL_PREFIX:PATH=$HOME/build-boringssl .. &&
make &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

libdecrepit.a

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

.travis.yml Outdated
@@ -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 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

git clone --depth=1 …

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

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea!

@bagder
Copy link
Member Author

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
Copy link
Member Author

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.

@mark-kubacki
Copy link
Contributor

mark-kubacki 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.

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
Copy link
Member Author

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.

@mark-kubacki
Copy link
Contributor

Good enough for me. Thanks!

@bagder
Copy link
Member Author

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 November 30, 2017 07:17
@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.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants