Push for 2.12 #47

reactormonk · 2016-12-07T14:31:28Z

Depends on Verizon/ermine-parser#2 and Verizon/sbt-rig#3, fixes #45

timperrett · 2016-12-09T18:00:47Z

@reactormonk alrighty! The correct version for the ermine dependency is 0.3.5 (obviously then there's the a versions as well). If you could kindly update the PR to use that, it should work.

timperrett · 2016-12-09T18:02:24Z

core/build.sbt

 resolvers ++= Seq(
  "scalaz.bintray" at "http://dl.bintray.com/scalaz/releases",
  "oncue.bintray" at "http://dl.bintray.com/oncue/releases"
 )

 libraryDependencies ++= {
  val ermineVersion =
-    if(scalazStreamVersion.value.startsWith("0.7")) "0.3.3a"
-    else "0.3.3"
+    if(List("0.7", "0.8").find(prefix => scalazStreamVersion.value.startsWith(prefix)).isDefined) "0.3.4a"


I'm not sure this is right. This will pull in the wrong version of scalaz... given there are only 0.7 and 0.8 builds, the else clause will never be reached. I think this can be taken out.

reactormonk · 2016-12-09T18:15:15Z

Oh god, you do it the other way round, a for the 7.1 version and nothing for the 7.2 version?

Yeah, should have seen that earlier: https://github.com/Verizon/ermine-parser/blob/master/project/CrossLibraryPlugin.scala#L23 Compare with: http://http4s.org/#getting-started

timperrett · 2016-12-09T18:23:45Z

@reactormonk the whole scalaz-stream a convention is terrible. The problem here is the ermine-parser is using multiple versions of scalaz-stream in the various builds, so we're having to infer the scalaz-version from the scalaz-stream version.

I just just checked with @djspiewak on the matter, and he said:

in 0.7, non-`a` refers to scalaz 7.0, while `a` refers to 7.1 
in 0.8, non-`a` is 7.1, and `a` refers to 7.2

reactormonk · 2016-12-09T18:36:34Z

Ah ok, your everyday clusterfuck.

Where is the ermine-parser 0.3.5? Can't find it on central.

timperrett · 2016-12-09T18:50:48Z

@reactormonk welcome to the party ;-) the various versions and their interop is a bit of a nightmare due to binary compatibility. Here's the artifacts on central: https://repo1.maven.org/maven2/io/verizon/ermine/parser_2.11/ - their search index takes a while to update it seems, but they are there (naturally the link is just for the 2.11 artifacts, but the 2.10 and 2.12 ones are also there)

reactormonk · 2016-12-09T19:23:49Z

The build hangs locally as well.

timperrett · 2016-12-09T19:30:01Z

@reactormonk that is...weird. It never used too. Let me try.

timperrett · 2016-12-09T19:35:51Z

@reactormonk building your PR locally I get:

Test.scala:150: ambiguous reference to overloaded definition,
[error] both method apply in object Configured of type (f: knobs.CfgValue => Option[scala.concurrent.duration.FiniteDuration])knobs.Configured[scala.concurrent.duration.FiniteDuration]
[error] and  method apply in object Configured of type (implicit evidence$1: knobs.Configured[scala.concurrent.duration.FiniteDuration])knobs.Configured[scala.concurrent.duration.FiniteDuration]
[error] match expected type ?
[error]       Configured[FiniteDuration].apply(CfgDuration(d)) == None
[error]                 ^
[error] /Users/v643874/repositories/verizon/oss/knobs/core/src/test/scala/knobs/Test.scala:156: ambiguous reference to overloaded definition,
[error] both method apply in object Configured of type (f: knobs.CfgValue => Option[scala.concurrent.duration.FiniteDuration])knobs.Configured[scala.concurrent.duration.FiniteDuration]
[error] and  method apply in object Configured of type (implicit evidence$1: knobs.Configured[scala.concurrent.duration.FiniteDuration])knobs.Configured[scala.concurrent.duration.FiniteDuration]
[error] match expected type ?
[error]     (Configured[FiniteDuration].apply(cfg): Option[Duration]) ?= Configured[Duration].apply(cfg)
[error]                ^
[error] /Users/v643874/repositories/verizon/oss/knobs/core/src/test/scala/knobs/Test.scala:156: ambiguous reference to overloaded definition,
[error] both method apply in object Configured of type (f: knobs.CfgValue => Option[scala.concurrent.duration.Duration])knobs.Configured[scala.concurrent.duration.Duration]
[error] and  method apply in object Configured of type (implicit evidence$1: knobs.Configured[scala.concurrent.duration.Duration])knobs.Configured[scala.concurrent.duration.Duration]
[error] match expected type ?
[error]     (Configured[FiniteDuration].apply(cfg): Option[Duration]) ?= Configured[Duration].apply(cfg)
[error]                                                                            ^
[error] three errors found

reactormonk · 2016-12-09T19:46:21Z

Only for 2.12 - will investigate.

reactormonk · 2016-12-09T23:10:09Z

It's probably http://www.scala-lang.org/news/2.12.0#sam-conversion-precedes-implicits

reactormonk · 2016-12-11T12:22:24Z

In preparing an SO, I figured that the two apply method get SAM'd - so their signature are the same. There's a few workarounds for that, but they're all gonna break something.

add another abstract method: will break custom instances
rename one of them to instance ala circe

timperrett · 2016-12-11T20:43:14Z

I'm ok with a breaking change provide the major version is incremented. Thanks for looking into this :-)

djspiewak · 2016-12-12T19:32:39Z

@reactormonk @timperrett I've PR'd a potential fix to your branch here. Quoting the explanation so you don't need to click through:

The problem, fundamentally, is that the compiler is resolving the implicit Configured instance and then attempting to resolving the typing on the subsequent application of the apply method. When it does that, SAM checking kicks in and it sees Configured as a valid instantiation of the second apply overload (the one which takes a function), and thus both branches of the overload are applicable at the call site. It's less confusing to see if you remove the implicit sugar.

Added workaround for SAM ambiguity on 2.12

Daenyth · 2016-12-15T14:44:59Z

Is there anything I can do to speed this along? I have a new codebase I've started with 2.12 and I'd like to use knobs with it.

timperrett · 2016-12-15T17:45:38Z

@Daenyth with the changes @reactormonk added, the builds hang, and its not clear why. master as it is currently, does not have this problem, so something in this PR is causing a hang, so we cannot merge as-is. If you would like to help out fixing this, that would be awesome

Daenyth · 2016-12-15T18:10:03Z

Looking at the diff I can only think it's something from one of

scalaz-stream version change
sbt-rig version change

Looking at the sbt-rig change, I'm going to guess scalaz-streams. I'll start looking at changelogs there.
@reactormonk I assume that's the first version for 2.12?

reactormonk · 2016-12-15T21:50:17Z

@Daenyth Yeah. http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22org.scalaz.stream%22%20AND%20a%3A%22scalaz-stream_2.12%22

…in 2.12

rhyskeepence · 2017-01-12T12:02:24Z

I had a quick look at this, as this is the final dependency preventing us from moving to 2.12.

I couldn't get to the bottom of the issue, but fundamentally, there appears to be a race condition between subscribing and updating a value, causing a deadlock in the zookeeper test. Adding a sleep between the subscribe and update (which, FWIW, the FileWatcherTest is also doing), appears to remove the deadlock.

PR for potential fix which might be worth considering to get a 2.12 release.

sleep to prevent deadlock in 2.12

timperrett · 2017-01-12T19:31:14Z

@reactormonk @rhyskeepence thanks for your continued contribution to this gents. We'll discuss internally and hopefully merge this now its passing. We might consider removing the zookeeper module, or moving it elsewhere, as we're not using it these days.

ppurang · 2017-01-20T09:24:42Z

knobs stands between us and 2.12.1 glory. Would love to have it today rather than tomorrow. Can we have #43 also included.

I had a go at this too - with ermine-parser 0.3.5a. Forcing dependency to scalaz 7.2.x leads to binary incompatibility:

Exception: java.lang.NoSuchMethodError: scalaz.Free$.point(Lscala/Function0;)Lscalaz/Free;

timperrett · 2017-01-20T18:02:51Z

@ppurang right now this PR needs rebasing because it wont merge.

ppurang · 2017-01-26T17:07:41Z

@timperrett could I do something to unblock this issue?

timperrett · 2017-01-26T18:07:16Z

@ppurang i want to migrate this to the same naming structure that recently got added to delorean to be explicit about the scalaz version being used. The a append convention is entirely broken, and now that we have quite a large matrix, i'd like it to be explicit. If you would be able to make that change it would be awesome!

slouc · 2017-02-17T15:10:05Z

Any updates on this?

SteveDraper · 2017-02-22T14:56:40Z

Knobs appears to also be our only dependency that blocks a move to 2.12 currently. Looking through this thread it seems a lot of work has already been done, and a significant blocker is Zookeeper related only. As has been observed above, most users of Knobs probably are not using the Zookeeper integration, so moving this to a separate module might be a good pragmatic decision. However if that's not the only blocker I guess its a bit academic...

Any likely ETA?

timperrett · 2017-02-22T15:32:14Z

Yes we are aware people are waiting for an official release. Me and @rossabaker are working on fixing the mess with the a appension and the classifiers. Will discuss internally today and post an ETA here later. Thanks for using Knobs :-)

timperrett · 2017-02-22T15:33:26Z

@SteveDraper the zookeeper issue was resolved, so thats no longer an issue.

rossabaker · 2017-02-23T03:12:44Z

I have extended this pull request with #51. It builds with the -scalaz-7.x convention, which we are adopting for all scalaz-based libraries.

Thanks to all for the contributions and interest in this effort.

timperrett · 2017-02-24T08:53:21Z

Huge thank you to @reactormonk @ppurang and @rhyskeepence for your persistence in this PR... and my sincere apologies for having had little time to make this a priority. With that frame, also thanks to @rossabaker for stepping up to get this done for everyone :-) We've factored the work done in this PR and a couple of other PRs into #51 and made the following choices:

All Verizon libraries will now use maven classifier as a discriminating factor for knowing which scalaz version it uses.
Knobs will stop supporting scalaz-stream 0.7 (we will upgrade our internal dependencies, as we wanted to do that anyway).
ermine-parser and sbt-rig already have the needed changes to make this release happen.

I've added a few small comments, and @rossabaker will be able to fix these up Friday, so then we can get a release done. Once again, thanks everyone for your interest in Knobs and contributions here. This took far longer than it should have :-D

All the best

Tim

reactormonk added 3 commits December 7, 2016 15:29

Push for 2.12

423736f

Bumped sbt-rig version

343ebd3

Added 2.12 build

6fba0f3

timperrett reviewed Dec 9, 2016

View reviewed changes

Bumped ermine to 0.3.5 and removed custom resolvers

5e0a923

Added workaround for SAM ambiguity on 2.12

f5faa79

Merge pull request #1 from djspiewak/feature/scala-2.12

586fae1

Added workaround for SAM ambiguity on 2.12

sleep between subscribe and config modification, preventing deadlock …

78e6744

…in 2.12

Merge pull request #2 from rhyskeepence/master

b06662e

sleep to prevent deadlock in 2.12

Merge branch 'master' into master

79da19b

timperrett mentioned this pull request Feb 22, 2017

Update to 2.12 #50

Closed

rossabaker mentioned this pull request Feb 23, 2017

Release for 2.12 #51

Merged

timperrett closed this Feb 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push for 2.12 #47

Push for 2.12 #47

reactormonk commented Dec 7, 2016 •

edited

Loading

timperrett commented Dec 9, 2016

timperrett Dec 9, 2016

reactormonk commented Dec 9, 2016 •

edited

Loading

timperrett commented Dec 9, 2016

reactormonk commented Dec 9, 2016

timperrett commented Dec 9, 2016 •

edited

Loading

reactormonk commented Dec 9, 2016

timperrett commented Dec 9, 2016

timperrett commented Dec 9, 2016

reactormonk commented Dec 9, 2016

reactormonk commented Dec 9, 2016

reactormonk commented Dec 11, 2016 •

edited

Loading

timperrett commented Dec 11, 2016

djspiewak commented Dec 12, 2016

Daenyth commented Dec 15, 2016

timperrett commented Dec 15, 2016

Daenyth commented Dec 15, 2016 •

edited

Loading

reactormonk commented Dec 15, 2016

rhyskeepence commented Jan 12, 2017

timperrett commented Jan 12, 2017

ppurang commented Jan 20, 2017

timperrett commented Jan 20, 2017

ppurang commented Jan 26, 2017

timperrett commented Jan 26, 2017

slouc commented Feb 17, 2017

SteveDraper commented Feb 22, 2017

timperrett commented Feb 22, 2017

timperrett commented Feb 22, 2017

rossabaker commented Feb 23, 2017

timperrett commented Feb 24, 2017

Push for 2.12 #47

Push for 2.12 #47

Conversation

reactormonk commented Dec 7, 2016 • edited Loading

timperrett commented Dec 9, 2016

timperrett Dec 9, 2016

Choose a reason for hiding this comment

reactormonk commented Dec 9, 2016 • edited Loading

timperrett commented Dec 9, 2016

reactormonk commented Dec 9, 2016

timperrett commented Dec 9, 2016 • edited Loading

reactormonk commented Dec 9, 2016

timperrett commented Dec 9, 2016

timperrett commented Dec 9, 2016

reactormonk commented Dec 9, 2016

reactormonk commented Dec 9, 2016

reactormonk commented Dec 11, 2016 • edited Loading

timperrett commented Dec 11, 2016

djspiewak commented Dec 12, 2016

Daenyth commented Dec 15, 2016

timperrett commented Dec 15, 2016

Daenyth commented Dec 15, 2016 • edited Loading

reactormonk commented Dec 15, 2016

rhyskeepence commented Jan 12, 2017

timperrett commented Jan 12, 2017

ppurang commented Jan 20, 2017

timperrett commented Jan 20, 2017

ppurang commented Jan 26, 2017

timperrett commented Jan 26, 2017

slouc commented Feb 17, 2017

SteveDraper commented Feb 22, 2017

timperrett commented Feb 22, 2017

timperrett commented Feb 22, 2017

rossabaker commented Feb 23, 2017

timperrett commented Feb 24, 2017

reactormonk commented Dec 7, 2016 •

edited

Loading

reactormonk commented Dec 9, 2016 •

edited

Loading

timperrett commented Dec 9, 2016 •

edited

Loading

reactormonk commented Dec 11, 2016 •

edited

Loading

Daenyth commented Dec 15, 2016 •

edited

Loading