Skip to content

MINOR: Update build.gradle and release.py to upload streams-scala_2.12#5368

Merged
rajinisivaram merged 4 commits intoapache:trunkfrom
rajinisivaram:MINOR-upload-streams-scala-2.12
Jul 19, 2018
Merged

MINOR: Update build.gradle and release.py to upload streams-scala_2.12#5368
rajinisivaram merged 4 commits intoapache:trunkfrom
rajinisivaram:MINOR-upload-streams-scala-2.12

Conversation

@rajinisivaram
Copy link
Copy Markdown
Contributor

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@rajinisivaram rajinisivaram requested a review from ijuma July 15, 2018 16:21
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

Hmm, why isn't uploadArchivesAll working? We should not need the 2_12 version anymore.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jul 15, 2018

For reference, Scala 2.12 was added to the *All commands when we dropped Java 7 support: e70a191#diff-6512f838e273b79676cac5f72456127f

Looks like the update to release.py was incomplete and the supposedly redundant upload archive line was not removed.

@rajinisivaram
Copy link
Copy Markdown
Contributor Author

@ijuma Thanks for the review. The various xxxAll tasks in build.gradle seem to explicitly run tasks for core, which is possibly why streams-scala didn't get uploaded for scala 2.12. I have removed the explicit 2.12 upload in release.py and updated build.gradle to run tasks for streams-scala as well wherever it was running tasks for core with different scala versions. Can you take another look? Thanks.

Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, just a nit.

Comment thread build.gradle Outdated
startParameter = project.getGradle().getStartParameter().newInstance()
startParameter.projectProperties += [scalaVersion: "${sv}"]
// core has a separate source jar for each scala version, streams-scala source is included
// in the single streams sources jar
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@guozhangwang @mjsax The behaviour with regards to the Scala source mentioned in the comment above is a bit weird.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we have a separate jar for each scala version as well for kafka-streams-scala?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ijuma @guozhangwang Sorry, this was an issue with the older version of gradle that I was testing with. Updated that and now it does generate separate source jars (I dont understand why). Updated build.gradle.

Comment thread build.gradle Outdated

tasks.create(name: "testConnect", dependsOn: connectPkgs.collect { it + ":test" }) {}
tasks.create(name: "testAll", dependsOn: withDefScalaVersions('test_core') + pkgs.collect { it + ":test" }) { }
tasks.create(name: "testAll", dependsOn: withDefScalaVersions('test_scala') + pkgs.collect { it + ":test" }) { }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should use camel case here and a few lines above for consistency with the other task names.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ijuma Thanks for the review, updated.

Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

The patch LGTM.

Comment thread release.py
if not user_ok("Going to build and upload mvn artifacts based on these settings:\n" + contents + '\nOK (y/n)?: '):
fail("Retry again later")
cmd("Building and uploading archives", "./gradlew uploadArchivesAll", cwd=kafka_dir, env=jdk8_env)
cmd("Building and uploading archives", "./gradlew uploadCoreArchives_2_12 -PscalaVersion=2.12", cwd=kafka_dir, env=jdk8_env)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just curious: seems we do not need this step even before this PR. Is that right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that should have been removed in a previous PR.

Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM

@rajinisivaram
Copy link
Copy Markdown
Contributor Author

retest this please

@rajinisivaram
Copy link
Copy Markdown
Contributor Author

Merging to trunk and 2.0.

@rajinisivaram rajinisivaram merged commit 10a0b8a into apache:trunk Jul 19, 2018
rajinisivaram added a commit that referenced this pull request Jul 19, 2018
#5368)

Reviewers: Ismael Juma <ismael@juma.me.uk>, Guozhang Wang <wangguoz@gmail.com>
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.

3 participants