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.

Sign up for GitHub

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

Jump to bottom

scrooge: Fixing Travis CI builds #196

Closed
wants to merge 5 commits into from
Closed

scrooge: Fixing Travis CI builds #196

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28b8983
scrooge: fixing Travis CI build
csaltos Jun 16, 2015
8fe5750
scrooge: optimizing Travis CI builds for non master branches
csaltos Jun 17, 2015
dc60240
scrooge: Fix Travis CI builds
csaltos Jun 17, 2015
5c389bb
scrooge: Fix Travis CI builds
csaltos Jun 17, 2015
a5a4002
scrooge: Adding external artifacts cache to the Travis CI configuration
csaltos Jun 17, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Expand Up @@ -7,4 +7,4 @@ jdk:
- oraclejdk7
- openjdk7

script: "./sbt ++$TRAVIS_SCALA_VERSION test"
script: ./build-support/bin/ci.sh
61 changes: 61 additions & 0 deletions build-support/bin/ci.sh
View file
Open in desktop
@@ -0,0 +1,61 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love putting this in build-support/bin, we already have a bin directory, can we use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. I will put it in the already existing bin directory.


Copy link
Contributor

Choose a reason for hiding this comment

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

set -e?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the way !!

function clone_repo {
local name=$1
local repo=$2
local branch=$3
git clone $repo || {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can simplify this by using git clone "$repo" --branch "$branch"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right ... I will go for it and change it at the script.

echo "Unable to clone $name from $repo"
exit 1
}
cd $name || {
echo "Unable to get $name from $repo"
exit 2
}
git checkout $branch || {
echo "Unable to checkout branch $branch for $name from $repo"
exit 3
}
cd ..
}

function publish_local {
local name=$1
local target=$2
cd $name || {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unlikely to fail, and if it does, set -e will help us out with it. I don't think we need the || check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point, will change it with set -e and -x too.

echo "Unable to get $name for publish local $target"
exit 4
}
../../../sbt ++$TRAVIS_SCALA_VERSION $target || {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use ./sbt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using the ./sbt included with finagle and util gives errors, exactly the error is ->

bad sbt-launch.jar. delete sbt-launch.jar and run ./sbt again.

I tried to delete de sbt-launch.jar just for curiosity but the error remains.

For a moment I though it was a local error on my machine but Travis CI gives the same error.

So, I just use the sbt included on scrooge.

Do you have the same error using ./sbt from finagle ?

I will try to figure out what the error is and we can discuss about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's very strange. That should only happen if you're in a directory which already has an sbt-launch.jar. Don't spend too much time on it, if you can't figure out why, just add a TODO.

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've found the error -> The SBTOPTS from Travis CI are clashing with the internal ./sbt

So I just added "unset SBTOPTS" to the .travis.yml file and it works now and actually finagle's .travis.yml file has the same "unset SBTOPTS" configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh oh !! ... the ./sbt of util is crashing at the JVM when using openjdk7 and scrooge sbt is working just fine. I will put a TODO by now.

echo "Unable to publish local $name with target $target"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is cool, but we could probably just publish-local everything. do you not want to do that because it's slow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was because it was slow, but on top of that it's because a couple of finagle projects got errors while building from develop, so I try to reduce the impact on finagle dependencies including the ones that specifically scrooge depends on. What do you think ?

exit 5
}
cd ..
}

function bootstrap_scrooge {
local current_branch=$(git rev-parse --abbrev-ref HEAD)
if [ "$current_branch" != "master" ]; then
mkdir -p build-support/tmp
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use mktemp instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duh for me !! ... you are right, that's even better (easier and cleaner). I will change it to mktemp instead.

cd build-support/tmp
# util
clone_repo util https://github.com/twitter/util.git develop
publish_local util publishLocal
# ostrich
clone_repo ostrich https://github.com/twitter/ostrich.git develop
publish_local ostrich publishLocal
# finagle
clone_repo finagle https://github.com/twitter/finagle.git develop
publish_local finagle finagle-core/publishLocal
publish_local finagle finagle-mux/publishLocal
publish_local finagle finagle-httpx/publishLocal
publish_local finagle finagle-thrift/publishLocal
publish_local finagle finagle-thriftmux/publishLocal
publish_local finagle finagle-ostrich4/publishLocal
cd ../..
fi
}

bootstrap_scrooge

./sbt ++$TRAVIS_SCALA_VERSION test
4 changes: 4 additions & 0 deletions project/Build.scala
View file
Open in desktop
Expand Up @@ -14,6 +14,10 @@ object Scrooge extends Build {
val suffix = if (branch == "master") "" else "-SNAPSHOT"

val libVersion = "3.18.1" + suffix

// For develop users: if you are using new features in util and finagle dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say "For users of the develop branch"? Seems a little clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it might be a good idea to recommend publishing Util and Finagle locally for anyone working on the develop branch—users may not have any idea whether Scrooge is using features that have been introduced since the last release, and if they guess wrong they may end up with weird errors (possibly at runtime).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. Here it goes !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ostrich is also a required local dependency when working with the develop branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I think that explanation is already included at https://github.com/twitter/scrooge/blob/master/README.md

// that are not yet published to Maven Central, you'll need to publish util and
// finagle locally
val utilVersion = "6.24.0" + suffix
val finagleVersion = "6.25.0" + suffix

Expand Down