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

+pro #15870 OSGi headers for experimental Streams and HTTP modules #17979

Merged
merged 2 commits into from Jul 14, 2015

Conversation

Projects
None yet
6 participants
@rkrzewski

rkrzewski commented Jul 13, 2015

No description provided.

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 13, 2015

Collaborator

Can one of the repo owners verify this patch?

Collaborator

akka-ci commented Jul 13, 2015

Can one of the repo owners verify this patch?

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jul 13, 2015

Member

refs #15870

Member

ktoso commented Jul 13, 2015

refs #15870

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jul 13, 2015

Member

OK TO TEST

Member

ktoso commented Jul 13, 2015

OK TO TEST

@akka-ci akka-ci added validating tested and removed validating labels Jul 13, 2015

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 13, 2015

Collaborator

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3192/

Collaborator

akka-ci commented Jul 13, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3192/

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 13, 2015

Collaborator

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3195/

Collaborator

akka-ci commented Jul 13, 2015

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3195/

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Jul 13, 2015

I have amended the commit with OSGi settings for akka-stream-testkit and akka-http-testkit
akka-http-testkit is problematic because it depends on scalatest. scalatest is available as a bundle, but a proorly constructed one: it has mandatory dependencies on a ton of stuff (jmock, easymock, mockito, selenium, ant and more) and some of the dependencies is not available as bundles, most notably ... junit and hamcrest.
Does it make sense to use akka-http-testkit without scalatest in the runtime classpath? Making the dependency optional would give the bundle a chance to be loaded.

rkrzewski commented Jul 13, 2015

I have amended the commit with OSGi settings for akka-stream-testkit and akka-http-testkit
akka-http-testkit is problematic because it depends on scalatest. scalatest is available as a bundle, but a proorly constructed one: it has mandatory dependencies on a ton of stuff (jmock, easymock, mockito, selenium, ant and more) and some of the dependencies is not available as bundles, most notably ... junit and hamcrest.
Does it make sense to use akka-http-testkit without scalatest in the runtime classpath? Making the dependency optional would give the bundle a chance to be loaded.

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Jul 13, 2015

merge conflict, will rebase.

rkrzewski commented Jul 13, 2015

merge conflict, will rebase.

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 13, 2015

Collaborator

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3196/

Collaborator

akka-ci commented Jul 13, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3196/

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Jul 13, 2015

There's one remaining problem: scala.xml needs to be imported with different version range when building against Scala 2.11+. I'd like do do something like this:

def scalaXmlImport: Seq[String] = 
  if(scalaVersion == "2.10") Nil
  else Seq(versionedImport("scala.xml.*", "1.0", "1.1"))

val httpXml = exports(Seq("akka.http.scaladsl.marshallers.xml"),
        imports = Seq(streamAndHttpImport("akka.stream.*"),
            streamAndHttpImport("akka.http.*")) ++ scalaXmlImport)

but I don't know the proper SBT idiom to do the scalaVersion == "2.10" part.

rkrzewski commented Jul 13, 2015

There's one remaining problem: scala.xml needs to be imported with different version range when building against Scala 2.11+. I'd like do do something like this:

def scalaXmlImport: Seq[String] = 
  if(scalaVersion == "2.10") Nil
  else Seq(versionedImport("scala.xml.*", "1.0", "1.1"))

val httpXml = exports(Seq("akka.http.scaladsl.marshallers.xml"),
        imports = Seq(streamAndHttpImport("akka.stream.*"),
            streamAndHttpImport("akka.http.*")) ++ scalaXmlImport)

but I don't know the proper SBT idiom to do the scalaVersion == "2.10" part.

@akka-ci akka-ci added validating and removed tested labels Jul 13, 2015

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Jul 13, 2015

Done. I verified locally that correct imports are generated both in 2.10 and 2.11 builds.
If you prefer those 2 commits squashed, let me know.

rkrzewski commented Jul 13, 2015

Done. I verified locally that correct imports are generated both in 2.10 and 2.11 builds.
If you prefer those 2 commits squashed, let me know.

@akka-ci akka-ci added tested and removed validating labels Jul 13, 2015

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 13, 2015

Collaborator

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3199/

Collaborator

akka-ci commented Jul 13, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3199/

+pro #15870 generate correct version range for scala.xml import on Sc…
…ala 2.11+

scala.xml was extracted into a separate module with independent relase cycle
starting with Scala 2.11.0. In order to generate correct OSGi bundles for
this environment, an import for scala.xml.* with appropriate version range
must be inserted before scala.* import in BND instructions.

@akka-ci akka-ci added validating and removed tested labels Jul 14, 2015

@@ -1428,11 +1458,17 @@ object AkkaBuild extends Build {
"com.google.protobuf")
def exports(packages: Seq[String] = Seq(), imports: Seq[String] = Nil) = osgiSettings ++ Seq(
OsgiKeys.importPackage := imports ++ defaultImports,
OsgiKeys.importPackage := imports,
OsgiKeys.importPackage <++= scalaVersion { v =>

This comment has been minimized.

@ktoso

ktoso Jul 14, 2015

Member

"2.10" is scalaBinaryVersion :)
Then matching on it is OK

@ktoso

ktoso Jul 14, 2015

Member

"2.10" is scalaBinaryVersion :)
Then matching on it is OK

This comment has been minimized.

@jrudolph

jrudolph Jul 14, 2015

Member

Are you sure this change is at the right place? This will add scala.xml to any package that uses exports, right?

@jrudolph

jrudolph Jul 14, 2015

Member

Are you sure this change is at the right place? This will add scala.xml to any package that uses exports, right?

This comment has been minimized.

@rkrzewski

rkrzewski Jul 14, 2015

@jrudolph no worries, BND is smart and will add it to Import-Package header only when actually needed. I do this in this place, because it needs to be in front of scala.* to be effective.

@ktoso should I change the condition to scalaBinaryVersion { v => if(v == "2.10") ... or should it stay as is? I actually nicked that from cpsPlugin settings declaration at line 170

@rkrzewski

rkrzewski Jul 14, 2015

@jrudolph no worries, BND is smart and will add it to Import-Package header only when actually needed. I do this in this place, because it needs to be in front of scala.* to be effective.

@ktoso should I change the condition to scalaBinaryVersion { v => if(v == "2.10") ... or should it stay as is? I actually nicked that from cpsPlugin settings declaration at line 170

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jul 14, 2015

Member

Looks great, thank you @rkrzewski!
After merging all other PRs I'll get to this one and verify we're not missing anything, then - release 1.0!

Member

ktoso commented Jul 14, 2015

Looks great, thank you @rkrzewski!
After merging all other PRs I'll get to this one and verify we're not missing anything, then - release 1.0!

@ktoso ktoso self-assigned this Jul 14, 2015

@akka-ci akka-ci added tested and removed validating labels Jul 14, 2015

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 14, 2015

Collaborator

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3203/

Collaborator

akka-ci commented Jul 14, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3203/

@ktoso ktoso added the t:stream label Jul 14, 2015

@2m

This comment has been minimized.

Show comment
Hide comment
@2m

2m Jul 14, 2015

Member

I wrote a quick script to verify that jars contain all of the compiled class files. So this LGTM.

Member

2m commented Jul 14, 2015

I wrote a quick script to verify that jars contain all of the compiled class files. So this LGTM.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jul 14, 2015

Member

thanks! That's super helpful, so confidently merging this.

Member

ktoso commented Jul 14, 2015

thanks! That's super helpful, so confidently merging this.

val httpCore = Nil // FIXME #15689
val httpTestKit = exports(Seq("akka.http.scaladsl.testkit.*", "akka.http.javaadsl.testkit.*"),

This comment has been minimized.

@ktoso

ktoso Jul 14, 2015

Member

excellent, thanks @rkrzewski

@ktoso

ktoso Jul 14, 2015

Member

excellent, thanks @rkrzewski

This comment has been minimized.

@jrudolph

jrudolph Aug 11, 2015

Member

Bam! javaadsl spoiled the show. Hard to spot, though...

@jrudolph

jrudolph Aug 11, 2015

Member

Bam! javaadsl spoiled the show. Hard to spot, though...

This comment has been minimized.

@ktoso

ktoso Aug 11, 2015

Member

aaaaargh :<

@ktoso

ktoso Aug 11, 2015

Member

aaaaargh :<

This comment has been minimized.

@rkrzewski

rkrzewski Aug 11, 2015

sorry about that :(

@rkrzewski

rkrzewski Aug 11, 2015

sorry about that :(

This comment has been minimized.

@jrudolph

jrudolph Aug 11, 2015

Member

No worries about that, this is so easy to get wrong. The problem is that there are no safeguards that would catch a problem like this.

@jrudolph

jrudolph Aug 11, 2015

Member

No worries about that, this is so easy to get wrong. The problem is that there are no safeguards that would catch a problem like this.

This comment has been minimized.

@ktoso

ktoso Aug 11, 2015

Member

Yeah, no worries - I mean, I reviewed and looked at this exact line and still missed the typo.
Will be fixed soon in 1.1 tho

@ktoso

ktoso Aug 11, 2015

Member

Yeah, no worries - I mean, I reviewed and looked at this exact line and still missed the typo.
Will be fixed soon in 1.1 tho

This comment has been minimized.

@rkrzewski

rkrzewski Aug 11, 2015

Hmm, shouldn't the script written by @2m have detected this problem?

Also, the verification script should check that the generated jar does not contain more classes than target/classes folder, because it's easy to accidentally re-export packages from an upstream dependency if a too broad Export declaration is used (which is completely valid, and occasionally useful in OSGi environment)

@rkrzewski

rkrzewski Aug 11, 2015

Hmm, shouldn't the script written by @2m have detected this problem?

Also, the verification script should check that the generated jar does not contain more classes than target/classes folder, because it's easy to accidentally re-export packages from an upstream dependency if a too broad Export declaration is used (which is completely valid, and occasionally useful in OSGi environment)

ktoso added a commit that referenced this pull request Jul 14, 2015

Merge pull request #17979 from rkrzewski/streams-http-osgi
+pro #15870 OSGi headers for experimental Streams and HTTP modules

@ktoso ktoso merged commit e135ebc into akka:release-2.3-dev Jul 14, 2015

2 checks passed

default Merged build finished.
Details
typesafe-cla-validator All users have signed the CLA
Details
@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski commented Jul 14, 2015

😎

ktoso added a commit to ktoso/akka that referenced this pull request Jan 11, 2016

Merge pull request #17979 from rkrzewski/streams-http-osgi
+pro #15870 OSGi headers for experimental Streams and HTTP modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment