typesafehub / conductr-lib

Other
8 stars 13 forks source link

Lagom 1.3.x flavor #127

Open markusjura opened 7 years ago

markusjura commented 7 years ago

Lagom 1.3.x is not binary compatible with previous Lagom versions.

When a Lagom project with version 1.2.2 uses the latest sbt-conductr version then the following error occurs when running the bundle:

2017-02-20T14:46:38Z Markuss-MacBook-Pro.local WARN  Reaper [bundleId=c7e2773d7c6241dbdefe3beac9954dfc, sourceThread=conductr-agent-akka.actor.default-dispatcher-34, akkaTimestamp=14:46:38.253UTC, akkaSource=akka://conductr-agent/user/reaper, sourceActorSystem=conductr-agent] - Exception in thread "main"
2017-02-20T14:46:38Z Markuss-MacBook-Pro.local WARN  Reaper [bundleId=c7e2773d7c6241dbdefe3beac9954dfc, sourceThread=conductr-agent-akka.actor.default-dispatcher-34, akkaTimestamp=14:46:38.253UTC, akkaSource=akka://conductr-agent/user/reaper, sourceActorSystem=conductr-agent] - Exception in thread "main"
2017-02-20T14:46:38Z Markuss-MacBook-Pro.local WARN  Reaper [bundleId=c7e2773d7c6241dbdefe3beac9954dfc, sourceThread=conductr-agent-akka.actor.default-dispatcher-34, akkaTimestamp=14:46:38.253UTC, akkaSource=akka://conductr-agent/user/reaper, sourceActorSystem=conductr-agent] - java.lang.NoClassDefFoundError: com/lightbend/lagom/internal/api/ServiceReader$

This is because sbt-conductr 2.2.4 brings in lagom1-conductr-bundle-lib which uses the Lagom version 1.3.0-RC1. This version is then used across the project because it is later than 1.2.2, i.e. 1.2.2 is evicted:

> show update
[info] Updating {file:/Users/mj/workspace/activator-lagom-java-chirper/}activity-stream-impl...
[info] Resolving com.lightbend.lagom#api-tools_2.11;1.2.2 ...
[info] Done updating.
[info] Update report:
[info]  Resolve time: 2769 ms, Download time: 32 ms, Download size: 0 bytes
[info]  bundle:
[info]  configuration:
[info]  universal:
[info]  compile:
[info]  com.typesafe.conductr:lagom1-java-conductr-bundle-lib_2.11
[info]      - 1.6.1
[info]          status: release
[info]          publicationDate: Tue Feb 14 20:01:12 CET 2017
[info]          resolver: sbt-chain
[info]          artifactResolver: sbt-chain
[info]          evicted: false
[info]          homepage: http://conductr.lightbend.com/
[info]          isDefault: false
[info]          configurations: default, compile, runtime, default(compile), master
[info]          licenses: (Apache-2.0,Some(https://www.apache.org/licenses/LICENSE-2.0.html))
[info]          callers: sample.chirper:activity-stream-impl_2.11:1.0-SNAPSHOT
[info]
[info]  com.lightbend.lagom:lagom-javadsl-client_2.11
[info]      - 1.3.0-RC1
[info]          status: release
[info]          publicationDate: Tue Jan 24 04:37:10 CET 2017
[info]          resolver: sbt-chain
[info]          artifactResolver: sbt-chain
[info]          evicted: false
[info]          homepage: http://www.lagomframework.com/
[info]          isDefault: false
[info]          configurations: compile, runtime(*), master(compile), runtime, compile(*), master
[info]          licenses: (Apache-2.0,Some(http://www.apache.org/licenses/LICENSE-2.0.html))
[info]          callers: com.lightbend.lagom:lagom-javadsl-server_2.11:1.2.2, com.typesafe.conductr:lagom1-java-conductr-bundle-lib_2.11:1.6.1
[info]      - 1.2.2
[info]          status: release
[info]          publicationDate: Thu Jan 12 11:35:59 CET 2017
[info]          resolver: sbt-chain
[info]          artifactResolver: sbt-chain
[info]          evicted: true
[info]          evictedData: latest-revision
[info]          homepage: http://www.lagomframework.com/
[info]          isDefault: false
[info]          configurations: compile, runtime(*), master(compile), runtime, compile(*), master
[info]          licenses: (Apache-2.0,Some(http://www.apache.org/licenses/LICENSE-2.0.html))
[info]          callers: com.lightbend.lagom:lagom-javadsl-server_2.11:1.2.2

Solution

In conductr-lib we need to create a new lagom13 flavor for conductr-bundle-lib and conductr-client-lib.

markusjura commented 7 years ago

@huntc @jroper Because you are sbt dependency cracks, can you please review this issue and especially the proposed solution.

Also, can we safely rename the existing lagom1-java-conductr-bundle-lib to lagom12-java-conductr-bundle-lib. Same counts for lagom1-scala-conductr-bundle-lib

jroper commented 7 years ago

Lagom 1.3 is binary compatible in its public APIs, the problem is the internal APIs are not binary compatible, which means when you mix different versions of Lagom together, it causes a problem. ConductR bundle lib should not be causing an upgrade of any Lagom library (or any library for that matter). I think (not 100%) if ConductR bundle lib used provided scoped dependencies, then it wouldn't cause an upgrade.

jroper commented 7 years ago

I've just done some research, and indeed provided is the scope that conductr-bundlelib should be using, probably for every dependency on the frameworks it provides support for. Provided dependencies are available at compile time, but are not transitive, so that means conductr-bundlelibs version of a dependency will have no impact on the project that it's being used with.

markusjura commented 7 years ago

Sounds good @jroper. I'll create a PR for that.

markusjura commented 7 years ago

Created the WIP PR https://github.com/typesafehub/conductr-lib/pull/129. I am currently stuck as descriped in the problem description of the PR.

huntc commented 7 years ago

@jroper

Lagom 1.3 is binary compatible in its public APIs, the problem is the internal APIs are not binary compatible, which means when you mix different versions of Lagom together, it causes a problem.

I'm confused about this. Lagom 1.x.x is supposed to be binary compatible for all x.x. We are not calling any internal API, so I'm unsure why conductr-lib is responsible for dealing with this particular issue. We've certainly not had to for the other toolkits and frameworks.

Is there anything that can be done in the 1.3 RC in order to resolve the issue?

ConductR bundle lib should not be causing an upgrade of any Lagom library (or any library for that matter).

I don't see why ConductR lib should be different than any other lib in this regard.

jroper commented 7 years ago

The problem is that when ConductR depends on Lagom 1.3, that transitively upgrades the users Lagom dependencies to 1.3. But only the ones that ConductR transitively depends on. The ones that it doesn't depend on, for example Lagom persistence, stay at Lagom 1.2. And Lagom persistence uses internal APIs that are not binary compatible between versions of Lagom from some of the Lagom libraries that the ConductR dependency has transitively upgraded. This is particularly a problem in the Lagom 1.2 to 1.3 upgrade, because internally Lagom has gone through a massive overhaul where things have been split and moved around and new internal artifacts introduced in order to support the Scala API. There's just no way you can expect that, for example, Lagom persistence 1.2 could ever work with Lagom core 1.3.

I don't see why ConductR lib should be different than any other lib in this regard.

It's not different. This is always a problem. Some libraries will just force you to upgrade to a minimum version of the version that they depend on. sbt always takes the most recent version when there are conflicts, so if ConductR lib depends on Lagom 1.3, then when sbt sees that ConductR depends on Lagom 1.3.0, and the user has depended on Lagom 1.2.2, it will resolve the conflict by selecting the most recent, so sbt will select 1.3.0, which consequently forces the user to upgrade to Lagom 1.3.0. And that's no problem, unless you want to let people use the same library with Lagom 1.0 (which you do), then it is a problem. There are two ways to allow that, one is to mark the dependencies as provided, which means that ConductR is making no assertion on what version of Lagom it requires, it just expects the user to provide a version of Lagom. The other is to say that ConductR depends on the earliest version of Lagom it is compatible with, in this case, 1.0.0. That way, when sbt sees that ConductR depends on 1.0.0, and the user depends on 1.2.2, sbt will select 1.2.2 because it's more recent, rather than 1.0.0.

huntc commented 7 years ago

Lagom 1.2 to 1.3 upgrade, because internally Lagom has gone through a massive overhaul where things have been split and moved around and new internal artifacts introduced in order to support the Scala API. There's just no way you can expect that, for example, Lagom persistence 1.2 could ever work with Lagom core 1.3.

I think that this statement highlights the problem. Lagom has had a major overhaul between 1.2 and 1.3. I think that our best approach with conductr-lib is therefore to have 1.2 and 1.3 flavours. There is no actual guarantee of binary compatibility between minor releases. Do you agree @jroper?

jroper commented 7 years ago

No I disagree. If you were using Lagom 1.0.0, and you added ConductR to your application, would you expect to end up using Lagom 1.2.2 without explicitly upgrading yourself? Because that is exactly what will happen if you go with that solution.

jroper commented 7 years ago

If you think it's reasonable ConductR triggers an upgrade from Lagom 1.0.0 to 1.2.2 without warning the user, then why worry about ConductR triggering an upgrade from 1.2.2 to 1.3.0? Why not just say "ConductR forces you to use 1.3.0, so that's what you have to use"?

huntc commented 7 years ago

No I disagree. If you were using Lagom 1.0.0, and you added ConductR to your application, would you expect to end up using Lagom 1.2.2 without explicitly upgrading yourself? Because that is exactly what will happen if you go with that solution.

YES I would expect that because I was under the impression that 1.x.x is binary compatible. Why wouldn't I want to upgrade to the latest?

huntc commented 7 years ago

Perhaps my understanding that 1.x.x was binary compatible for all of x.x was ill founded though.

huntc commented 7 years ago

If you think it's reasonable ConductR triggers an upgrade from Lagom 1.0.0 to 1.2.2 without warning the user, then why worry about ConductR triggering an upgrade from 1.2.2 to 1.3.0? Why not just say "ConductR forces you to use 1.3.0, so that's what you have to use"?

Because a user's build will break... and it isn't obvious why until docs are read.

jroper commented 7 years ago

This is the case for every single library out there, we see this exact same problem in patch versions of Akka 2.4, you can't use akka-actor 2.4.16 with akka-remote 2.4.0, for example, because akka-remote depends on internal akka-actor APIs that have changed in binary incompatible ways. We see exactly the same thing in Netty, Play's the same, I bet you conductr-bundle-lib is the same. This isn't a Lagom thing, it's a normal expectation that you have to upgrade the entire library, not partially upgrade.

jroper commented 7 years ago

And it matters to understand this because it's not just 1.2.x to 1.3.x that will have this problem, it's 1.3.0 to 1.3.1, 1.3.1 to 1.3.2, everything, our internal APIs are up for grabs when it comes to binary compatibility. Running lagom-javadsl-server 1.3.1 with lagom-javadsl-client 1.3.2 together on the classpath will never be a supported configuration, just as running two different versions of Play libraries, or two different versions of Akka libraries, at the same time, is not supported, and it's something that we will never test binary compatibility of and never try and ensure works. So, if we're saying that it's ok for conductr-bundle-lib to force an upgrade, that's fine, they are binary compatible, the users should have no problem doing it, but the user must fully upgrade, they can't mix and match different parts of Lagom with different versions. So it's just a matter of documenting to say that the user has to upgrade to at least Lagom 1.3.0 in order to use this.

huntc commented 7 years ago

Thanks James. This makes complete sense now.

The underlying problem here is that there is no notion in build tools of what constitutes a "family" of dependencies that must correlate in their versions.

Could the Lagom plugin cross check that the "family" of Lagom versions is correct and thus warn the user if not?

jroper commented 7 years ago

Maybe - we could probably hook into the dependencyClasspath task and make sure that every Lagom library version matches the Lagom version in the plugin.

markusjura commented 7 years ago

Would that also work for maven?

jroper commented 7 years ago

Don't even get me started with Maven. The nice thing about Maven is that you can force the versions of transitive dependencies by using dependencyManagement. The unnice thing about Maven is that you are forced to force the versions of transitive dependencies by using dependencyManagement, because the algorithm that Maven uses to resolve conflicts (nearest to root of dependency tree wins) will, more often than not, result in the version that you didn't want being picked, so unless you are 100% explicit with the version of every transitive dependency in dependencyManagement, you're opening yourself up to a world of pain. But apparently Maven users love Maven and this is a perfectly reasonable feature that they live with and it doesn't bother them.

jroper commented 7 years ago

But all that said, what we can do in Lagom is provide a dependency management import, which will allow users to import a dependency management section that we generate into the build, thus giving us the ability to completely control their transitive dependencies. That will solve this issue for Maven users.

markusjura commented 7 years ago

I'll let you @jroper and @huntc decide which solution we are taking on due to the fact that you are both in the same timezone. Maybe it is best to sleep over that :-)

In the meantime I am implementing the sbt-conductr workaround as described here in the section Intermediate solution: https://github.com/typesafehub/conductr-lib/pull/129#issuecomment-281635444

This should give us some breathing time to implement a final solution.

jroper commented 7 years ago

We'll solve this in Lagom by implementing https://github.com/lagom/lagom/issues/529. This should actually allow users to use conductr-bundle-lib without changing their Lagom version, even if conductr-bundle-lib depends on a newer version of Lagom.