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

Enables Hydra on cats build #2848

Merged
merged 14 commits into from
May 23, 2019
Merged

Enables Hydra on cats build #2848

merged 14 commits into from
May 23, 2019

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented May 17, 2019

Thanks to @dotta 's generous help and donation, we can enable hydra on Cats build to potentially speed up build as well as to obtain compilation metrics. The metrics can be viewed here https://dashboard.triplequote.com
The code change here is simply moving the traits defined in a package object file into their own file. This is due to a hydra issue that triplequote is working on.

@kailuowang kailuowang closed this May 17, 2019
@kailuowang kailuowang reopened this May 17, 2019
@kailuowang kailuowang changed the title [WIP] Enables Hydra on cats build Enables Hydra on cats build May 17, 2019
@codecov-io
Copy link

codecov-io commented May 17, 2019

Codecov Report

Merging #2848 into master will decrease coverage by 0.31%.
The diff coverage is 96.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2848      +/-   ##
=========================================
- Coverage   94.21%   93.9%   -0.32%     
=========================================
  Files         368     368              
  Lines        6944    6984      +40     
  Branches      301     312      +11     
=========================================
+ Hits         6542    6558      +16     
- Misses        402     426      +24
Impacted Files Coverage Δ
...ats/kernel/instances/FiniteDurationInstances.scala 100% <ø> (ø)
.../scala/cats/kernel/instances/StringInstances.scala 100% <100%> (ø)
.../scala/cats/kernel/instances/SymbolInstances.scala 100% <100%> (ø)
...in/scala/cats/kernel/instances/UUIDInstances.scala 100% <100%> (ø)
...ain/scala/cats/kernel/instances/IntInstances.scala 100% <100%> (ø)
...in/scala/cats/kernel/instances/CharInstances.scala 100% <100%> (ø)
...cala/cats/kernel/instances/FunctionInstances.scala 100% <100%> (ø)
...cala/cats/kernel/instances/DurationInstances.scala 100% <100%> (ø)
...n/scala/cats/kernel/instances/QueueInstances.scala 100% <100%> (ø)
...in/scala/cats/kernel/instances/ListInstances.scala 100% <100%> (ø)
... and 43 more

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 c81513a...5426c67. Read the comment docs.

djspiewak
djspiewak previously approved these changes May 17, 2019
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

I wasn't able to review each code change manually, but I trust mima to catch anything horrible. Exciting!

@dotta
Copy link
Contributor

dotta commented May 18, 2019

One thing I noticed while moving bits around is that there is no package object finiteDuration​ inside cats/kernel/instances/finiteDuration/package.scala. Was it on purpose? I'm just raising this in case it was an oversight and you'd like to add it.

@kailuowang
Copy link
Contributor Author

Yeah, that's an oversight. I will add it.

@dotta
Copy link
Contributor

dotta commented May 19, 2019

A quick note to inform you that in case you are considering adopting sbt 1.3.0-rc1, we have found an incompatibility with our sbt-hydra plugin, preventing the two to work together. The good news is that we have a fix ready, and a new release will likely be announced on Monday. I'll comment again here when the release is out so that you can upgrade the sbt-hydra version.

@dotta
Copy link
Contributor

dotta commented May 21, 2019

As promised, here is a quick update about sbt 1.3.0-RC1. There is a regression in sbt affecting the sbt-hydra plugin (the hydra-bridge sources JAR aren't downloaded because a resolver is missing). We opened a ticket to track this problem (sbt/sbt#4712).

Unless you wanted to upgrade to sbt 1.3.0-RC1 right away, this PR should not be blocked the linked issue.

@kailuowang
Copy link
Contributor Author

turns out that finiteDuration as a name of a package is not scalastyle compliant. I am going to resolve that in a different PR.

@kailuowang kailuowang requested a review from LukaJCB May 23, 2019 16:09
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.

Looks good, thanks Kai! :)

@kailuowang kailuowang merged commit 8069e09 into master May 23, 2019
@kailuowang kailuowang deleted the hydra branch May 24, 2019 13:42
@travisbrown
Copy link
Contributor

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.

@kailuowang kailuowang added this to the 2.0.0-M2 milestone Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants