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

Upgrade to sbt 1 #338

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

jeffboutotte
Copy link

@jeffboutotte jeffboutotte commented Jul 22, 2019

Upgrade the build to sbt 1.2.8 in order to support the release of an sbt 1.x version of this plugin.

Bump the version to 2.0.0-SNAPSHOT and remove most things deprecated in the 1.x releases of the plugin.

Parallelize build across scripted tests so build time on Travis isn't pushing the 50 minute limit.

Huge props to @soc for doing a bunch of work to get this started.

@jeffboutotte
Copy link
Author

jeffboutotte commented Jul 22, 2019

Things to do still (before a 2.0.0 release at least):

  • Remove deprecated flavorOf from android package object. I still needed this for the android-test-kit test.
  • Fix the Travis build
  • Update sbt-android-protify plugin to support sbt 1.x. One of the tests depends on this, but the plugin itself does not.
  • Update docs (completely slipped my mind)

@jeffboutotte
Copy link
Author

@pfn I found some time over the weekend to pull this thread. This is a pretty large change, so no rush on a review. I'd appreciate feedback from anyone else interested as well!

@sideeffffect
Copy link
Contributor

@jeffboutotte the main problem at this moment, is that CI is broken, and has been for a long time
I tried to fix it a while ago, and made some progress, but ultimately ran out of ideas #332 :(
do you think you could look into what's wrong on Travis side? perhaps that's what we need to fix as the first thing before proceeding to other things

@jeffboutotte
Copy link
Author

@sideeffffect let me take a look - I don't have much experience with travis, but I'm happy to learn what I can :D I'll take a look and see if I can figure something out. This week is a bit busy for me, but I'll see if I can get some time in the evenings or over the weekend to dig in.

@jeffboutotte
Copy link
Author

jeffboutotte commented Sep 18, 2019

@sideeffffect I ended up with some time to dig in this evening and ended up figuring it out. My branch is here if you want to pull my commit into your PR to keep things there since that's where I started from.

@sideeffffect
Copy link
Contributor

great news @jeffboutotte ! 🎉
the PR #332 fixing Travis CI has been merged 💪
do you think you can now pick up work on this PR to upgrade to sbt 1.x?

@jeffboutotte
Copy link
Author

@sideeffffect thanks for letting me know! I just merged in the travis fixes and pushed up a change. We'll see if travis likes it.

@jeffboutotte
Copy link
Author

Looks like the tests on travis are hovering right around that 50 minute timeout. There may be some things we can do to improve build time, I'll see what I can find out...

@shea-hawkins
Copy link

@jeffboutotte Let me know if there's anything I can help with.

@jeffboutotte
Copy link
Author

@shea-hawkins thanks for the offer - I'd love some help! Things have been a bit busy the past couple of weeks so I haven't had much time to dig in. Since we're pretty close to the timeout my initial thought was to leverage the travis build matrix feature to parallelize enough to get the tests under. One natural split might be to run the android-sdk-plugin tests and gradle-build tests in parallel. Another possibility is to dig into the performance of individual tests. I'd be curious to see if there's one thing in particular that we do across tests that's slow - a little performance bump for each of the tests could be a combined big performance gain. That second one is just an early idea more than anything, I haven't run any experiments to test the theory or see if there's anything we can do there so the travis one may be more immediately actionable.

@jeffboutotte
Copy link
Author

Actually, here are some recommendations from the sbt reference manual - https://www.scala-sbt.org/1.x/docs/Travis-CI-with-sbt.html#More+things. It looks like we could even possibly parallelize the android-sdk-plugin tests as well.

@jeffboutotte
Copy link
Author

@shea-hawkins I just looked into the performance of individual tests quickly and it looks like it's just sbt scripted that's slow. I think that means you can ignore my second point above since a lot of that performance is out of our control. I think parallelizing as described in that reference manual is probably the way to go.

@jeffboutotte
Copy link
Author

Looks like splitting scripted tests as 1of2/2of2 isn't supported for sbt 0.13.x. I split the tests in this PR instead of its own since splitting and parallelizing the android-sdk-plugin tests will make the biggest impact on cutting down the build time on Travis. We'll see how the build goes with those changes or if there's more iterating to do...

@jeffboutotte
Copy link
Author

I think (hope) that last commit should get the build working. The sbt plugin automatically installs the targeted android platform but the gradle build plugin does not (since it's meant to port a gradle build to sbt). I think it's a safe assumption that a gradle build works before someone tries to use this plugin to convert from gradle to sbt. A side effect of parallelizing the build is that the gradle plugin tests don't have any sbt tests run beforehand that install the various versions of the android platform those tests used. The last commit updates the gradle plugin tests to use the android version that travis installs, that way the gradle build has the version it needs to work already.

@sideeffffect
Copy link
Contributor

@jeffboutotte build is green, those are great news! 🎉
btw if you just need to trigger a Travis build, you can close and immediately re-open a PR

@neobrain
Copy link

neobrain commented Mar 4, 2020

Very much looking forward to this change :)

Just curious, what's left to be done before this PR can be merged? Is it advisable to use this branch in production over master in the meantime?

@sideeffffect
Copy link
Contributor

@jeffboutotte is there anything left? Or can this be merged?

@jeffboutotte
Copy link
Author

I think this PR is ready to be merged, just waiting on a review. Anyone who wants to review and give feedback or ask questions is definitely welcome and encouraged to do so!

As for an official 2.0.0 version (not a snapshot) I think there are a couple of outstanding items that would be good follow ons to this PR. They don't really have to do with sbt 1.x, but are minor things we would probably want before publishing a v2 of this plugin. They are listed in my first comment on this PR:

  • Update the docs. This might want to be done right before an official 2.0.0 release so the docs can continue to reflect the current version until v2 is ready to release.
  • Since we're bumping the major version, it's probably a good time to remove some of the deprecated things. I managed to get most of them out except for the flavorOf function in the android package object. I couldn't quite figure out how to get one of the tests that uses it to work without it.
  • Update the sbt-android-protify plugin to support sbt 1.x. One of the tests depends on this, but the plugin itself does not. These tests don't run on travis anyways, but it may be confusing locally if the tests don't work. The sbt-android-protify plugin depends on this plugin, so maybe we wait until after 2.0.0 is released and leverage that version from sbt-android-protify first.

@sideeffffect
Copy link
Contributor

the people who can merge this are @soc, @pfn, @taig and @dant3
hopefully one of them will find the time to do so

@soc
Copy link
Member

soc commented Mar 5, 2020

I'm not involved in Scala anymore; given the size of this change I think @pfn would be the best person to review it. Maybe try pinging him on gitter?

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.

5 participants