You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/12/14 12:37:02 UTC
[GitHub] [iceberg] racevedoo opened a new pull request #3743: build: apply publishing config after project evaluation
racevedoo opened a new pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743
Since we refactored the spark projects to have separate folders/projects
per spark version (in 811af43), gradle was evaluating the maven publish
plugin config (in `deploy.gradle`) before it applied the `shadowJar`
configs required for spark runtime projects, causing the generated POM
to include dependencies when it shouldn't (as we're publishing a shadow
jar).
This fixes it by delaying the maven publishing config to
`afterEvaluate`, when gradle has already applied the shadowJar plugin
for projects.
Fixes #6320
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] racevedoo commented on pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
racevedoo commented on pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#issuecomment-993498958
@rdblue I found the root cause of the issue we discussed in #3642 (the pom generation issue). I believe this is the proper fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] racevedoo commented on pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
racevedoo commented on pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#issuecomment-993865027
> Awesome. Thank you @racevedoo! I had looked at this a little bit, but had no idea what the cause was.
No worries! I had to dig pretty deep to find this hehehe. Anything I must do before this gets merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#issuecomment-993683824
Awesome. Thank you @racevedoo! I had looked at this a little bit, but had no idea what the cause was.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#issuecomment-994142179
I was waiting for CI checks to pass. I also validated this manually and the pom files look correct again. I think this is ready to merge when Jack is satisfied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#issuecomment-994117952
I think everyone approves, this is super critical contribution, thank you! There were just some nitpicking, @racevedoo up to you if you would like to clean those up. I will merge after some time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#issuecomment-994158245
I realized I can just remove the extra newline through the github UI. I'll merge this. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#discussion_r769109064
##########
File path: deploy.gradle
##########
@@ -24,105 +24,108 @@ if (project.hasProperty('release') && jdkVersion != '8') {
subprojects {
apply plugin: 'maven-publish'
apply plugin: 'signing'
+ afterEvaluate {
- task sourceJar(type: Jar, dependsOn: classes) {
- classifier = 'sources'
- from sourceSets.main.allSource
- group 'build'
- }
+ task sourceJar(type: Jar, dependsOn: classes) {
+ classifier = 'sources'
+ from sourceSets.main.allSource
+ group 'build'
+ }
- task javadocJar(type: Jar, dependsOn: javadoc) {
- classifier = 'javadoc'
- from javadoc.destinationDir
- group 'build'
- }
+ task javadocJar(type: Jar, dependsOn: javadoc) {
+ classifier = 'javadoc'
+ from javadoc.destinationDir
+ group 'build'
+ }
- task testJar(type: Jar) {
- archiveClassifier = 'tests'
- from sourceSets.test.output
- }
+ task testJar(type: Jar) {
+ archiveClassifier = 'tests'
+ from sourceSets.test.output
+ }
- artifacts {
- archives sourceJar
- archives javadocJar
- archives testJar
- testArtifacts testJar
- }
+ artifacts {
+ archives sourceJar
+ archives javadocJar
+ archives testJar
+ testArtifacts testJar
+ }
- // add LICENSE and NOTICE
- [jar, sourceJar, javadocJar, testJar].each { task ->
- task.from(rootDir) {
- include 'LICENSE'
- include 'NOTICE'
+ // add LICENSE and NOTICE
+ [jar, sourceJar, javadocJar, testJar].each { task ->
+ task.from(rootDir) {
+ include 'LICENSE'
+ include 'NOTICE'
+ }
}
- }
- publishing {
- publications {
- apache(MavenPublication) {
- if (tasks.matching({task -> task.name == 'shadowJar'}).isEmpty()) {
- from components.java
- } else {
- project.shadow.component(it)
- }
+ publishing {
+ publications {
+ apache(MavenPublication) {
+ if (tasks.matching({task -> task.name == 'shadowJar'}).isEmpty()) {
+ from components.java
+ } else {
+ project.shadow.component(it)
+ }
- artifact sourceJar
- artifact javadocJar
- artifact testJar
+ artifact sourceJar
+ artifact javadocJar
+ artifact testJar
- versionMapping {
- allVariants {
- fromResolutionResult()
+ versionMapping {
+ allVariants {
+ fromResolutionResult()
+ }
}
- }
- groupId = 'org.apache.iceberg'
- pom {
- name = 'Apache Iceberg'
- description = 'A table format for huge analytic datasets'
- url = 'https://iceberg.apache.org'
- licenses {
- license {
- name = 'The Apache Software License, Version 2.0'
- url = 'http://www.apache.org/licenses/LICENSE-2.0.txt'
+ groupId = 'org.apache.iceberg'
+ pom {
+ name = 'Apache Iceberg'
+ description = 'A table format for huge analytic datasets'
+ url = 'https://iceberg.apache.org'
+ licenses {
+ license {
+ name = 'The Apache Software License, Version 2.0'
+ url = 'http://www.apache.org/licenses/LICENSE-2.0.txt'
+ }
}
- }
- mailingLists {
- mailingList {
- name = 'Dev Mailing List'
- post = 'dev@iceberg.apache.org'
- subscribe = 'dev-subscribe@iceberg.apache.org'
- unsubscribe = 'dev-unsubscribe@iceberg.apache.org'
+ mailingLists {
+ mailingList {
+ name = 'Dev Mailing List'
+ post = 'dev@iceberg.apache.org'
+ subscribe = 'dev-subscribe@iceberg.apache.org'
+ unsubscribe = 'dev-unsubscribe@iceberg.apache.org'
+ }
+ }
+ issueManagement {
+ system = 'GitHub'
+ url = 'https://github.com/apache/iceberg/issues'
}
- }
- issueManagement {
- system = 'GitHub'
- url = 'https://github.com/apache/iceberg/issues'
}
}
}
- }
- repositories {
- maven {
- credentials {
- username project.hasProperty('mavenUser') ? "$mavenUser" : ""
- password project.hasProperty('mavenPassword') ? "$mavenPassword" : ""
+ repositories {
+ maven {
+ credentials {
+ username project.hasProperty('mavenUser') ? "$mavenUser" : ""
+ password project.hasProperty('mavenPassword') ? "$mavenPassword" : ""
+ }
+ // upload to the releases repository using ./gradlew -Prelease publish
+ def apacheSnapshotsRepoUrl = 'https://repository.apache.org/content/repositories/snapshots'
+ def apacheReleasesRepoUrl = 'https://repository.apache.org/service/local/staging/deploy/maven2'
+ def snapshotsRepoUrl = project.hasProperty('mavenSnapshotsRepo') ? "$mavenSnapshotsRepo" : "$apacheSnapshotsRepoUrl"
+ def releasesRepoUrl = project.hasProperty('mavenReleasesRepo') ? "$mavenReleasesRepo" : "$apacheReleasesRepoUrl"
+ url = project.hasProperty('release') ? releasesRepoUrl : snapshotsRepoUrl
}
- // upload to the releases repository using ./gradlew -Prelease publish
- def apacheSnapshotsRepoUrl = 'https://repository.apache.org/content/repositories/snapshots'
- def apacheReleasesRepoUrl = 'https://repository.apache.org/service/local/staging/deploy/maven2'
- def snapshotsRepoUrl = project.hasProperty('mavenSnapshotsRepo') ? "$mavenSnapshotsRepo" : "$apacheSnapshotsRepoUrl"
- def releasesRepoUrl = project.hasProperty('mavenReleasesRepo') ? "$mavenReleasesRepo" : "$apacheReleasesRepoUrl"
- url = project.hasProperty('release') ? releasesRepoUrl : snapshotsRepoUrl
}
}
- }
- if (project.hasProperty('release')) {
- signing {
- useGpgCmd()
- sign publishing.publications.apache
+ if (project.hasProperty('release')) {
+ signing {
+ useGpgCmd()
+ sign publishing.publications.apache
+ }
}
+
Review comment:
nit: extra space
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#discussion_r769018497
##########
File path: deploy.gradle
##########
@@ -24,105 +24,108 @@ if (project.hasProperty('release') && jdkVersion != '8') {
subprojects {
apply plugin: 'maven-publish'
apply plugin: 'signing'
+ afterEvaluate {
- task sourceJar(type: Jar, dependsOn: classes) {
- classifier = 'sources'
- from sourceSets.main.allSource
- group 'build'
- }
+ task sourceJar(type: Jar, dependsOn: classes) {
+ classifier = 'sources'
+ from sourceSets.main.allSource
+ group 'build'
+ }
- task javadocJar(type: Jar, dependsOn: javadoc) {
- classifier = 'javadoc'
- from javadoc.destinationDir
- group 'build'
- }
+ task javadocJar(type: Jar, dependsOn: javadoc) {
+ classifier = 'javadoc'
+ from javadoc.destinationDir
+ group 'build'
+ }
- task testJar(type: Jar) {
- archiveClassifier = 'tests'
- from sourceSets.test.output
- }
+ task testJar(type: Jar) {
+ archiveClassifier = 'tests'
+ from sourceSets.test.output
+ }
- artifacts {
- archives sourceJar
- archives javadocJar
- archives testJar
- testArtifacts testJar
- }
+ artifacts {
+ archives sourceJar
+ archives javadocJar
+ archives testJar
+ testArtifacts testJar
+ }
- // add LICENSE and NOTICE
- [jar, sourceJar, javadocJar, testJar].each { task ->
- task.from(rootDir) {
- include 'LICENSE'
- include 'NOTICE'
+ // add LICENSE and NOTICE
+ [jar, sourceJar, javadocJar, testJar].each { task ->
Review comment:
Small nit / learning opportunity: There is a trailing white space after the `->`. I think that's what's making this diff look different than the other ones (that show that they are leading space changes only).
Generally speaking, the smaller the diff change the easier it is for people to cherry-pick and backport into their own forks. It's probably not as big of a deal for `deploy.gradle`, but I thought I'd take this opportunity to let you know as you get more involved with the project. It would likely be something to keep in mind when updating the actual code though. 😄
Usually, the goal is to simply keep the number of changes as small as possible so that if people `git cherry-pick`, they're less likely to have issues with the diff applying correctly. This file doesn't merit that level of scrutiny imho (though I currently don't maintain a fork).
That said, there's probably no major reason to change it. In some of the existing stuff in the file, there's a terminal space afterwards and some there is not. So this has just been for knowledge sake (if you weren't already aware). 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#discussion_r769018497
##########
File path: deploy.gradle
##########
@@ -24,105 +24,108 @@ if (project.hasProperty('release') && jdkVersion != '8') {
subprojects {
apply plugin: 'maven-publish'
apply plugin: 'signing'
+ afterEvaluate {
- task sourceJar(type: Jar, dependsOn: classes) {
- classifier = 'sources'
- from sourceSets.main.allSource
- group 'build'
- }
+ task sourceJar(type: Jar, dependsOn: classes) {
+ classifier = 'sources'
+ from sourceSets.main.allSource
+ group 'build'
+ }
- task javadocJar(type: Jar, dependsOn: javadoc) {
- classifier = 'javadoc'
- from javadoc.destinationDir
- group 'build'
- }
+ task javadocJar(type: Jar, dependsOn: javadoc) {
+ classifier = 'javadoc'
+ from javadoc.destinationDir
+ group 'build'
+ }
- task testJar(type: Jar) {
- archiveClassifier = 'tests'
- from sourceSets.test.output
- }
+ task testJar(type: Jar) {
+ archiveClassifier = 'tests'
+ from sourceSets.test.output
+ }
- artifacts {
- archives sourceJar
- archives javadocJar
- archives testJar
- testArtifacts testJar
- }
+ artifacts {
+ archives sourceJar
+ archives javadocJar
+ archives testJar
+ testArtifacts testJar
+ }
- // add LICENSE and NOTICE
- [jar, sourceJar, javadocJar, testJar].each { task ->
- task.from(rootDir) {
- include 'LICENSE'
- include 'NOTICE'
+ // add LICENSE and NOTICE
+ [jar, sourceJar, javadocJar, testJar].each { task ->
Review comment:
Small nit / learning opportunity: There is a trailing white space after the `->`. I think that's what's making this diff look different than the other ones (that show that they are leading space changes only).
Generally speaking, the smaller the diff change the easier it is for people to cherry-pick and backport into their own forks. It's probably not as big of a deal for `deploy.gradle`, but I thought I'd take this opportunity to let you know as you get more involved with the project. 😄
Usually, the goal is to simply keep the number of changes as small as possible so that if people `git cherry-pick`, they're less likely to have issues with the diff applying correctly. This file doesn't merit that level of scrutiny imho (though I currently don't maintain a fork).
That said, there's probably no major reason to change it. In some of the existing stuff in the file, there's a terminal space afterwards and some there is not. So this has just been for knowledge sake (if you weren't already aware). 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#issuecomment-993950694
> No worries! I had to dig pretty deep to find this hehehe.
Thank you so much for this deep dive @racevedoo! It's greatly appreciated.
> Anything I must do before this gets merged?
I don't think so. If it's been approved by a PMC member (like @/rdblue) or even a committer, they're most likely just giving it more time for people to take a look for themselves and give any feedback required before some committer comes and merges it.
So the only thing you'd need to do would be respond to questions or concerns that people bring up in the mean time 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3743: build: apply publishing config after project evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3743:
URL: https://github.com/apache/iceberg/pull/3743#discussion_r769129703
##########
File path: deploy.gradle
##########
@@ -24,105 +24,108 @@ if (project.hasProperty('release') && jdkVersion != '8') {
subprojects {
apply plugin: 'maven-publish'
apply plugin: 'signing'
+ afterEvaluate {
- task sourceJar(type: Jar, dependsOn: classes) {
- classifier = 'sources'
- from sourceSets.main.allSource
- group 'build'
- }
+ task sourceJar(type: Jar, dependsOn: classes) {
+ classifier = 'sources'
+ from sourceSets.main.allSource
+ group 'build'
+ }
- task javadocJar(type: Jar, dependsOn: javadoc) {
- classifier = 'javadoc'
- from javadoc.destinationDir
- group 'build'
- }
+ task javadocJar(type: Jar, dependsOn: javadoc) {
+ classifier = 'javadoc'
+ from javadoc.destinationDir
+ group 'build'
+ }
- task testJar(type: Jar) {
- archiveClassifier = 'tests'
- from sourceSets.test.output
- }
+ task testJar(type: Jar) {
+ archiveClassifier = 'tests'
+ from sourceSets.test.output
+ }
- artifacts {
- archives sourceJar
- archives javadocJar
- archives testJar
- testArtifacts testJar
- }
+ artifacts {
+ archives sourceJar
+ archives javadocJar
+ archives testJar
+ testArtifacts testJar
+ }
- // add LICENSE and NOTICE
- [jar, sourceJar, javadocJar, testJar].each { task ->
- task.from(rootDir) {
- include 'LICENSE'
- include 'NOTICE'
+ // add LICENSE and NOTICE
+ [jar, sourceJar, javadocJar, testJar].each { task ->
Review comment:
I don't think that this was added by this PR. I think everything in the block was indented with no other change (at least, from ignore whitespace diff).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org