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

Remove sbt-hydra for now #2871

Merged
merged 1 commit into from
Jun 1, 2019

Conversation

travisbrown
Copy link
Contributor

See #2870 and my comment on #2848:

I wish this change had been put forward for more public discussion before being merged. I personally didn't hear anything about this before seeing all the tweets about it after it was done.

I think there are many good reasons to avoid requiring a closed-source sbt plugin to run the Cats build, and that's even if it had been tested thoroughly and we had established that it wasn't likely to cause problems. As it is right now running sbt prePR from a clean checkout crashes with this error on both my Mac laptop and Linux desktop, and it's completely impossible to investigate because sbt-hydra is just a binary from a commercial repo.

I appreciate Triplequote's generosity, but the Cats build already has plenty of sharp edges, especially for new contributors, and I really don't think throwing a WIP closed-source sbt plugin into the mix was a good idea.

If we're going to use a closed-source sbt plugin in the Cats build, I think there's a pretty clear case for it being gated behind an environmental variable (Guillaume Martres pointed out this example for sbt-dotty). This PR doesn't do that—it just disables the plugin for now so that when we tell people to run sbt prePR they don't get an opaque error message.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Environment variable sounds like a better solution to be too 👍

@codecov-io
Copy link

codecov-io commented Jun 1, 2019

Codecov Report

Merging #2871 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2871   +/-   ##
=======================================
  Coverage   94.21%   94.21%           
=======================================
  Files         368      368           
  Lines        6948     6948           
  Branches      307      308    +1     
=======================================
  Hits         6546     6546           
  Misses        402      402

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9b9a28...3b8c981. Read the comment docs.

@travisbrown travisbrown merged commit 52a4cd4 into typelevel:master Jun 1, 2019
@kailuowang
Copy link
Contributor

I totally agree that we shouldn't force contributors to use hydra.
Hydra is only used on cats build on main repo branches (i.e, it is not used in PR builds or when run locally)
sbt-hydra 2.1.4 was doing just that. I think all we need to do is to revert back to sbt-hydra 2.1.4

@travisbrown
Copy link
Contributor Author

@kailuowang The objection isn't just to Hydra being required, it's sbt-hydra, which is also not open source. If it's behind a gate and not the default, I don't personally have a problem with it being in the build, but I don't think it should be required.

@kailuowang kailuowang modified the milestones: 2.0.0-M2, 2.0.0-M3 Jun 4, 2019
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