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

Strip sbt-plugins, deprecated / EOL frameworks + modules and cross-compile 2.11/2.12 #225

Merged
merged 23 commits into from
Sep 12, 2019

Conversation

rreas
Copy link
Contributor

@rreas rreas commented Sep 10, 2019

Pretty aggressive change set here that tries to balance doing the minimal to compile on 2.12. Here I am trying to reduce this "common" code to actual "common" code that is or can be used in nearly all projects with low dependency coupling. All to be released under 2.0.0+ versioning.

Summary of changes are:

  1. Remove the indexing and webapp subprojects -- These use the old spray framework (not spray-json). This framework has been EOL'ed and is not ever going to work with 2.12. Further, these modules are not "common" in the sense that all projects need them.
  2. Remove all Akka code (test support + Guice) -- Again, we are not building only webapps. This cover just a few files and projects that need it can resurrect it in a different, shared repository or directly inline (or just never upgrade common).
  3. Remove Version + ComponentId -- Coupled to plugin support for these modules from sbt-plugins. Quickest way forward was to just remove them. A cursory search says few (if any) active (non-archived) projects are using these. Again, code can be mastered in projects that need it.
  4. Move dependencies (i.e., copy) from sbt-plugins to this code base.
  5. Use SBT 1.2.8
  6. Cross-compile for both 2.11 and 2.12. 2.13 does not work yet due to changes in collections and this PR is large enough that I decided to avoid exploring 2.13 further.
  7. Drops all the scala formatting things... These will need to be re-added after this change is approved (or just omitted forever).

Next steps:

  1. Add to CI with "+" targets that operate across versions
  2. Fix CI
  3. Possibly release the 2.0 series and tag the old 1.x series
  4. Update release process following notes from set-release about cross-compiling

@rreas rreas requested review from mjlangan, rodneykinney and schmmd and removed request for mjlangan September 10, 2019 21:05
Copy link
Member

@schmmd schmmd left a comment

Choose a reason for hiding this comment

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

Looks great! It's a big PR but it's easy as it's mostly deletions--the best type of PR. LMK how else I can help.

@@ -1,68 +1,74 @@
import Dependencies._

lazy val scala211 = "2.11.12"
Copy link
Member

Choose a reason for hiding this comment

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

Why did you make these lazy vals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly just following the conventions in the SBT documentation. Probably not strictly necessary here because they are used immediately after, however, an example of an unused val is the scala213 val (for now).

<developer>
<id>allenai-dev-role</id>
<name>Allen Institute for Artificial Intelligence</name>
<email>dev-role@allenai.org</email>
Copy link
Member

Choose a reason for hiding this comment

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

You might want to check whether this address goes anywhere. @dplessas would know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will find out, thanks.

@rodneykinney
Copy link
Member

So much cleaning up!

  • Remove webapp/indexing/Akka projects
  • Cross-compile
  • Move dependencies

Things I'm not sure about, that we should check before removing the dependency on the plugins:

  • Do we have tools for releasing allenai-common to bintray?
  • Do our auto-format pre-commit hooks still work, and sbt formatCheckStrict?

If we have CI that checks formatting and a release process at least documented in a .md file, then I'm good with this.

@schmmd
Copy link
Member

schmmd commented Sep 10, 2019

I think we did releases with a bintray plugin. I saw that @rreas left in a BinTray plugin--so we might be OK.

Docs for releasing using BinTray: https://github.com/allenai/common#releasing-new-versions

@rreas
Copy link
Contributor Author

rreas commented Sep 11, 2019

For release, I'm going to slightly update the process to have the actual release be less automated through CI since we release so rarely. I will also remove "Boss: Michael" from the Readme unless there are any objections @schmmd 😄 in addition to a few other tweaks.

@schmmd
Copy link
Member

schmmd commented Sep 11, 2019

@rreas no objections ;-)

At some point several years ago someone thought we should have a primary POC on repos and they wanted to call it the "boss". It was pretty short lived ;-)

@rreas
Copy link
Contributor Author

rreas commented Sep 11, 2019

@rodneykinney Can you re-review?

I am using a slightly newer formatter (Scalafmt) in this repository that provides the fail-on-would-reformat functionality out of the box and seems in general to be a bit more active than scalariform.

The CI check is sbt scalafmtCheckAll which includes src, test, it. To apply changes, it's a simple sbt scalafmtAll. For now, I have omitted scalastyle as well per our conversation. My hope is that scalafmt can unify the workflow.

Copy link
Member

@rodneykinney rodneykinney left a comment

Choose a reason for hiding this comment

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

Woot. I like the fact that we're trying out CircleCI. If we like it, we can use it for other S2 CI checks, and free up some resources on s2build.

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.

3 participants