Skip to content

open-api: Build runtime jar for test fixture #11279

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.

Sign up for GitHub

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

Merged
Fokko merged 1 commit into from
Nov 6, 2024
Merged

open-api: Build runtime jar for test fixture #11279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,9 @@ project(':iceberg-snowflake') {

project(':iceberg-open-api') {
apply plugin: 'java-test-fixtures'
apply plugin: 'com.gradleup.shadow'

build.dependsOn shadowJar

dependencies {
testImplementation project(':iceberg-api')
Expand All @@ -967,11 +970,9 @@ project(':iceberg-open-api') {
testFixturesImplementation project(':iceberg-gcp')
testFixturesImplementation project(':iceberg-azure')
testFixturesImplementation(libs.hadoop3.common) {
exclude group: 'log4j'
Copy link
Member Author

@ajantha-bhat ajantha-bhat Nov 5, 2024

Choose a reason for hiding this comment

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

Added because of the warning while brining up the runtime jar.

exclude group: 'org.slf4j'
exclude group: 'ch.qos.reload4j'
exclude group: 'org.apache.avro', module: 'avro'
exclude group: 'com.fasterxml.woodstox'
Copy link
Member Author

@ajantha-bhat ajantha-bhat Nov 5, 2024

Choose a reason for hiding this comment

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

Added these because of the error while brining up the runtime jar that class not found.

exclude group: 'com.google.guava'
exclude group: 'com.google.protobuf'
exclude group: 'org.apache.curator'
Expand All @@ -980,7 +981,6 @@ project(':iceberg-open-api') {
exclude group: 'org.apache.hadoop', module: 'hadoop-auth'
exclude group: 'org.apache.commons', module: 'commons-configuration2'
exclude group: 'org.apache.hadoop.thirdparty', module: 'hadoop-shaded-protobuf_3_7'
exclude group: 'org.codehaus.woodstox'
exclude group: 'org.eclipse.jetty'
}
testFixturesImplementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
Expand Down Expand Up @@ -1014,6 +1014,28 @@ project(':iceberg-open-api') {
recommend.set(true)
}
check.dependsOn('validateRESTCatalogSpec')

shadowJar {
archiveBaseName.set("iceberg-open-api-test-fixtures-runtime")
Copy link
Member Author

@ajantha-bhat ajantha-bhat Oct 8, 2024

Choose a reason for hiding this comment

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

Double check the naming. LGTM.

archiveClassifier.set(null)
configurations = [project.configurations.testFixturesRuntimeClasspath]
from sourceSets.testFixtures.output
zip64 true

// include the LICENSE and NOTICE files for the runtime Jar
from(projectDir) {
include 'LICENSE'
Copy link
Contributor

@danielcweeks danielcweeks Oct 9, 2024

Choose a reason for hiding this comment

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

We're actually packaging up more in this runtime than what's identified in the top-level LICENSE/NOTICE files, so we need to make sure that everything is captured.

Copy link
Member Author

@ajantha-bhat ajantha-bhat Oct 11, 2024

Choose a reason for hiding this comment

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

Do we have to do this manually or we use the standard tool for this?

I don't think we have a standard guidelines for this in "CONTRIBUTING.md". Can we add this?

Copy link
Member Author

@ajantha-bhat ajantha-bhat Oct 14, 2024

Choose a reason for hiding this comment

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

Used https://github.com/jk1/Gradle-License-Report to generate License and manually updated the notice for them.

Please check again.

Copy link
Member Author

@ajantha-bhat ajantha-bhat Oct 14, 2024

Choose a reason for hiding this comment

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

cc: @jbonofre

Copy link
Contributor

@danielcweeks danielcweeks Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just spot checking the NOTICE, but it doesn't include the Kite notice from the Iceberg NOTICE file. @bryanck had some way of generating the license/notice files that included transitive dependencies (which is required by ASF).

Copy link
Member Author

@ajantha-bhat ajantha-bhat Oct 17, 2024

Choose a reason for hiding this comment

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

@bryanck: Could you please help me here? I tried the same plugin that you mentioned in your PR here. I used the configuration as testFixturesRuntimeClasspath, because it is a shadow jar for test fixture source sets.

Also, We need to add the standard way in "contributing.md" for new users to follow it.

Copy link
Contributor

@bryanck bryanck Oct 17, 2024

Choose a reason for hiding this comment

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

I used the same plugin, but for both the notices and licenses. Also, I appended to the root license and notice files, the root notice has the Kite notice.

Copy link
Contributor

@bryanck bryanck Oct 17, 2024

Choose a reason for hiding this comment

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

@ajantha-bhat I can generate them if you want, I created a custom renderer for the plugin. I'm hoping to contribute that at some point so this is automated.

Copy link
Member Author

@ajantha-bhat ajantha-bhat Oct 17, 2024

Choose a reason for hiding this comment

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

I can also try manually appending the root files contents.

It only generates licenses, how did you generate the notice from that plugin?

Copy link
Contributor

@bryanck bryanck Oct 17, 2024

Choose a reason for hiding this comment

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

It can generate both, but I added some logic to my custom renderer to dedupe the notices.

include 'NOTICE'
}

manifest {
attributes 'Main-Class': 'org.apache.iceberg.rest.RESTCatalogServer'
}
}

jar {
Copy link
Member Author

@ajantha-bhat ajantha-bhat Oct 8, 2024

Choose a reason for hiding this comment

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

It was an empty jar since no source code. Hence, disabled it.

enabled = false
}
}

@Memoized
Expand Down
1 change: 1 addition & 0 deletions deploy.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ subprojects {
} else if (isOpenApi) {
artifact testJar
artifact testFixturesJar
artifact shadowJar
} else {
if (tasks.matching({task -> task.name == 'shadowJar'}).isEmpty()) {
from components.java
Expand Down
Loading