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

[BEAM-6178] Adding beam-sdks-java-bom, adding exportJavadoc flag for applyJavaNature #7197

Merged
merged 6 commits into from Dec 11, 2018

Conversation

Projects
None yet
2 participants
@garrettjonesgoogle
Copy link
Contributor

commented Dec 4, 2018

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---
@garrettjonesgoogle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

@swegner swegner self-requested a review Dec 6, 2018

@swegner

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

FYI, there are some merge conflicts. I will get started on the review, but before we can merge please resolve conflicts by either merge in master or rebase your branch on top. (git fetch origin && git rebase origin/master)

@garrettjonesgoogle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

sigh it was clean 2 days ago. I will resolve the conflicts.

@swegner
Copy link
Contributor

left a comment

If I understand correctly, this produces a new Maven artifact beam-sdks-java-bom which declares dependencyManagement dependencies for all Beam artifacts at the same version.

Is this useful? It seems straightforward for Beam consumers to define a single <beam.version> property in their POM for all Beam dependencies-- we do this in our archetype project. I would imagine it would be more useful to have version declarations for Beam's direct or transitive dependencies.

Show resolved Hide resolved build.gradle Outdated
Show resolved Hide resolved sdks/java/javadoc/build.gradle Outdated
Show resolved Hide resolved runners/samza/build.gradle
Show resolved Hide resolved sdks/java/bom/build.gradle
Show resolved Hide resolved sdks/java/bom/build.gradle Outdated
Show resolved Hide resolved sdks/java/bom/build.gradle Outdated
Show resolved Hide resolved sdks/java/bom/build.gradle
Show resolved Hide resolved sdks/java/bom/build.gradle
}

afterEvaluate {
// We can't use the `publishing` section from applyJavaNature because

This comment has been minimized.

Copy link
@swegner

swegner Dec 6, 2018

Contributor

Same comment as above-- this is a lot of redundancy and I worry that as they are maintained over time the implementations will drift. Can we refactor the main publishing config to have the necessary hooks for this task?

This comment has been minimized.

Copy link
@garrettjonesgoogle

garrettjonesgoogle Dec 7, 2018

Author Contributor

I played around with that a little but kept hitting evaluation ordering problems. I can try to take another crack at it.

This comment has been minimized.

Copy link
@garrettjonesgoogle

garrettjonesgoogle Dec 7, 2018

Author Contributor

I figured out a way to share the repositories declaration between BeamModulePlugin.groovy and this build.gradle.

// instead of the generated one.
publishing {
publications {
mavenJava(MavenPublication) {

This comment has been minimized.

Copy link
@swegner

swegner Dec 6, 2018

Contributor

Should consuming this BOM become the recommended way to depend on Beam? If so, perhaps we should implement it in the maven-archetypes. The archetype projects also get continuous testing with nightly releases, so that would help ensure our produced BOM is valid.

This comment has been minimized.

Copy link
@garrettjonesgoogle

garrettjonesgoogle Dec 7, 2018

Author Contributor

I think it should become the recommended method. I was thinking that the archetypes could be modified in a following PR. (FYI I tried it out on a project generated from an archetype locally and it worked great.)

This comment has been minimized.

Copy link
@swegner

swegner Dec 11, 2018

Contributor

Sounds good; please follow-up in a subsequent PR.

@garrettjonesgoogle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

Your high-level understanding is correct. In isolation, using a beam version property works just fine. The problem is when you want to set up another BOM that specifies the modules of Beam. In that case, it's not sustainable for that aggregated BOM to specify each module of Beam; it's much more sustainable for Beam to generate a BOM with Beam's module versions, which adds dependency declarations automatically when new modules are added to Beam, and then have the aggregated BOM just import the Beam BOM. We are trying to set up such an aggregator BOM at https://github.com/GoogleCloudPlatform/cloud-opensource-java/blob/master/boms/cloud-oss-bom/pom.xml .

@garrettjonesgoogle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

@swegner PTAL

I'm not sure why "Run Java PreCommit" is failing - looks like a flaky build system issue?

13:42:13 Failed to notify build listener.
13:42:13 > unable to create new native thread
@swegner

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

Run Python PreCommit

@swegner

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

Run Java PreCommit

@swegner

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

LGTM, looks like all feedback is addressed and tests are passing. I'll squash and merge.

@swegner swegner merged commit bfd1be9 into apache:master Dec 11, 2018

11 checks passed

CommunityMetrics ("Run CommunityMetrics PreCommit") SUCCESS
Details
Go ("Run Go PreCommit") SUCCESS
Details
Java ("Run Java PreCommit") SUCCESS
Details
JavaPortabilityApi ("Run JavaPortabilityApi PreCommit") SUCCESS
Details
Java_Examples_Dataflow ("Run Java_Examples_Dataflow PreCommit") SUCCESS
Details
Portable_Python ("Run Portable_Python PreCommit") SUCCESS
Details
Python ("Run Python PreCommit") SUCCESS
Details
RAT ("Run RAT PreCommit") SUCCESS
Details
Spotless ("Run Spotless PreCommit") SUCCESS
Details
Website ("Run Website PreCommit") SUCCESS
Details
Website_Stage_GCS ("Run Website_Stage_GCS PreCommit") SUCCESS
Details

garrettjonesgoogle added a commit to garrettjonesgoogle/beam that referenced this pull request Dec 12, 2018

swegner added a commit that referenced this pull request Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.