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 2022/04/26 23:57:04 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
kbendick opened a new pull request, #4638:
URL: https://github.com/apache/iceberg/pull/4638
## Purpose
As part of our journey to the 1.0 release, we'd like to have stability / binary compatibility guarantees for the `api` package.
## Added
Adds a gradle plugin, that's currently still actively maintained, for the `Revapi` tool, for checking API and ABI compatibility and ensuring that any breaks are caught during `gradlew check` (or `gradlew revapi`).
It also allows for allowing overrides and setting binary compatibility checks against different versions / tags.
Info can be found here: https://github.com/palantir/gradle-revapi
## Using
A generated override file is added that allows us to accept breaking changes and optionally denote why they're allowed.
This plugin requires that we point to some sort of base (to check binary / API compatibility from). I pointed it to 0.13.1 for this PR.
With this patch, if `./gradlew check` is run, it fails because it can't determine the correct upstream version to compare against.
So I ran the following to generate the config file in `./palantir/revapi.yml`.
```
./gradlew iceberg-api:revapiVersionOverride --replacement-version 0.13.1
```
Please feel free to take a look.
## Why this plugin
1. MiMa doesn't support Gradle
2. This plugin is actively maintained and we already use several palantir projects in our build / linter infrastructure.
3. The simplicity of the generated file.
4. The fact that this adds additional gradle tasks, but also runs during the `check` phase so that little to none of our CI needs to change.
Anybody who is interested, please take a look, play around and see how you like it.
cc @rdblue @danielcweeks @nastra @jackye1995 @snazy
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4638:
URL: https://github.com/apache/iceberg/pull/4638
--
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] KarlManong commented on pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
KarlManong commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1146736632
Sorry.
I create an issue https://github.com/apache/iceberg/issues/4959
--
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] KarlManong commented on pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
KarlManong commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1146720920
I do think it caused by https://github.com/palantir/gradle-revapi/issues/550
--
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 #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1110360226
The available tasks:
```bash
$ ./gradlew tasks --all | grep rev
iceberg-api:revapi
iceberg-api:revapiAcceptAllBreaks
iceberg-api:revapiAcceptBreak
iceberg-api:revapiAnalyze
iceberg-api:revapiVersionOverride
```
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861420454
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
+ org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
Review Comment:
The plug-in works by comparing the previous version with the current code, by fetching the previous version from maven.
The previous version is inferred based on the most recent tag, which for the master branch is `release-base-0.xx` normally.
However, that’s not an artifact available on maven.
So this sets the “old version” to compare for breaking changes to `apache-iceberg-0.13.1` from maven.
See here: https://github.com/palantir/gradle-revapi#version-overrides
--
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] KarlManong commented on pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
KarlManong commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1144541343
Can't run build on branch master 9ab94f87de036c9cd91cf8353906a576b4a516ff.
```
$ ./gradlew build -x test -x integrationTest
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 59821 100 59821 0 0 61926 0 --:--:-- --:--:-- --:--:-- 61862
Downloading https://services.gradle.org/distributions/gradle-7.4.2-bin.zip
...........10%...........20%...........30%...........40%...........50%...........60%...........70%...........80%...........90%...........100%
Welcome to Gradle 7.4.2!
Here are the highlights of this release:
- Aggregated test and JaCoCo reports
- Marking additional test source directories as tests in IntelliJ
- Support for Adoptium JDKs in Java toolchains
For more details see https://docs.gradle.org/7.4.2/release-notes.html
Starting a Gradle Daemon (subsequent builds will be faster)
FAILURE: Build failed with an exception.
* What went wrong:
Could not determine the dependencies of task ':iceberg-api:revapiAnalyze'.
> Failed to query the value of task ':iceberg-api:revapiAnalyze' property 'oldApiJars'.
> Failed to query the value of extension 'revapi' property 'oldVersions'.
> Failed running command:
Command:[]
Exit code: 128
Stdout:
Stderr:fatal: 没有标签能描述 '7ff467106431cdd8715aeec9b5f58458dc5d2102'。
尝试 --always，或者创建一些标签。
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
* Get more help at https://help.gradle.org
```
What did I miss? @kbendick
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861419166
##########
dev/.rat-excludes:
##########
@@ -9,6 +9,7 @@ build
.rat-excludes
LICENSE
NOTICE
+revapi.yml
Review Comment:
The reason I didn’t add it is because this file is autogenerated.
I can try adding it, forcing an update to the file, and then seeing if it handles the header well.
--
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 #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1110360590
Sample output from running `./gradlew check.
```
----------------------------------------------------------------------------------------------------
java.class.defaultSerializationChanged: The default serialization ID for the class has changed. This means that the new version of the class is not deserializable from the byte stream of a serialized old class.
old: class org.apache.iceberg.SortOrder
new: class org.apache.iceberg.SortOrder
SOURCE: EQUIVALENT, BINARY: EQUIVALENT, SEMANTIC: BREAKING
From old archive: iceberg-api-0.13.1.jar
From new archive: iceberg-api-0.14.0-SNAPSHOT.jar
If this is an acceptable break that will not harm your users, you can ignore it in future runs like so for:
* Just this break:
./gradlew :iceberg-api:revapiAcceptBreak --justification "{why this break is ok}" \
--code "java.class.defaultSerializationChanged" \
--old "class org.apache.iceberg.SortOrder" \
--new "class org.apache.iceberg.SortOrder"
* All breaks in this project:
./gradlew :iceberg-api:revapiAcceptAllBreaks --justification "{why this break is ok}"
* All breaks in all projects:
./gradlew revapiAcceptAllBreaks --justification "{why this break is ok}"
----------------------------------------------------------------------------------------------------
```
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861428232
##########
dev/.rat-excludes:
##########
@@ -9,6 +9,7 @@ build
.rat-excludes
LICENSE
NOTICE
+revapi.yml
Review Comment:
Unfortunately, it will have to go in the `.rat-excludes` file.
I added the header, then ran `./gradlew iceberg-api:revapiAcceptAllBreaks --justification "Testing YAML comment on top of file"` to cause an update to the file, and the resulting generated file removed the header comment.
So unless we want to manually re-enter the license header every time we make a chance to the file, it needs to be excluded.
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861429244
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
+ org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
Review Comment:
Here's the documentation on the actual underlying revapi tool: https://revapi.org/revapi-site/main/index.html
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861420954
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
+ org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
Review Comment:
Also the `Configuration` section has a succinct description of how it works: https://github.com/palantir/gradle-revapi#configuration
“””
gradle-revapi should work out of the box for most uses cases once applied.
By default it compares against the previous version of the jar from the project it is applied in by finding the last tag using git describe.
However, if you need to need to override the artifact to compare against, you can do so.
“””
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1146734780
Oh sorry. I see you do have a release-base.
I think maybe your gradle / maven dependency revolution (possibly in a fork) isn’t able to fetch the dependencies from the old version. Maybe a Firewall issue or maven / gradle configuration difference? Like I said, open an issue as a closed old PR is not the best place to discuss this.
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861417902
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
+ org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
Review Comment:
What is this doing?
--
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] danielcweeks commented on pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1111328182
I really like this approach due to the simplicity and how well it integrates into the current build process.
+1 (will wait on other feedback)
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r865209937
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
Review Comment:
This is marking it as 0.13.0 because of the output of `git describe`. I believe if we pushed a tag for `release-base-0.13.1`, it would use that instead.
However, it seems we can use an override in the `build.gradle` file instead: https://github.com/palantir/gradle-revapi#configuration
Let me try that.
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1126793778
But TLDR - We don’t have to add anything new to CI, this runs during gradle check.
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r865186643
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
+ org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
Review Comment:
Looking through their source code, the `.palantir` directory is hardcoded: https://github.com/kbendick/gradle-revapi/blob/09039d215efbd41e25b6743409a12ceb22ca5d75/src/main/java/com/palantir/gradle/revapi/RevapiPlugin.java#L176-L178
We could either add a comment somewhere in the Gradle build file or possibly try to open a PR upstream.
That said, nobody should have to update the config file manually. It's all done via `gradlew` commands which autogenerates the file and updates it.
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1117837683
Here's a gist comparing the output of 0.13.0 API module to the current 0.14.0-SNAPSHOT: https://gist.github.com/kbendick/b9543ac8498bb06815a8c4e993f5c969
This was generated by running `./gradlew revapi` from this branch.
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r863324441
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
Review Comment:
For the upstream version, I agree it should be `0.13.0`, which is the tag you're using. But should this mark it as 0.13.1? What happens if we make the branch 0.13.0 instead?
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r873061188
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
Review Comment:
I think we want to compare against the last minor release right now. Later, we'll want to compare to the last major release.
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861428524
##########
dev/.rat-excludes:
##########
@@ -9,6 +9,7 @@ build
.rat-excludes
LICENSE
NOTICE
+revapi.yml
Review Comment:
Here's part of the diff after forcing a chance in the file:
```
diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml
index 03fc8d427..f310c4923 100644
--- a/.palantir/revapi.yml
+++ b/.palantir/revapi.yml
@@ -1,21 +1,85 @@
-#
-# Licensed to the Apache Software Foundation (ASF) under one
-# or more contributor license agreements. See the NOTICE file
-# distributed with this work for additional information
-# regarding copyright ownership. The ASF licenses this file
-# to you under the Apache License, Version 2.0 (the
-# "License"); you may not use this file except in compliance
-# with the License. You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing,
-# software distributed under the License is distributed on an
-# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-# KIND, either express or implied. See the License for the
-# specific language governing permissions and limitations
-# under the License.
-#
-
versionOverrides:
org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
+acceptedBreaks:
+ release-base-0.13.0:
+ org.apache.iceberg:iceberg-api:
+ - code: "java.class.defaultSerializationChanged"
+ old: "class org.apache.iceberg.PartitionSpec"
+ new: "class org.apache.iceberg.PartitionSpec"
+ justification: "Testing YAML comment on top of file"
+ - code: "java.class.defaultSerializationChanged"
+ old: "class org.apache.iceberg.Schema"
+ new: "class org.apache.iceberg.Schema"
+ justification: "Testing YAML comment on top of file"
```
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1112898207
Another thought: This runs as part of `gradlew check`, so we don’t technically need to update CI.
However, we might want to add this as its CI step eventually.
The logic being that this check is relatively inexpensive, and so it could be placed first alongside the PR labeler. Then, if API compatibility fails, we could not run all of the expensive tests.
I wouldn’t add it as a standalone check just yet because it will get run as part of `gradle check`, but something to think about.
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r865298932
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
Review Comment:
If we tagged a `release-base-0.13.1`, we'd also be able to compare that from within the same file.
I left the full output comparison in a gist. https://gist.github.com/kbendick/b9543ac8498bb06815a8c4e993f5c969
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r865285051
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
Review Comment:
Setting it to 0.13.0 then compares the output of current master (or the generated code from this branch) to the `0.13.0` artifact.
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r863324173
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
+ org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
Review Comment:
Is it possible to move this to a different directory? I isn't very obvious what to fix if this is in `.palantir`.
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r863323890
##########
dev/.rat-excludes:
##########
@@ -9,6 +9,7 @@ build
.rat-excludes
LICENSE
NOTICE
+revapi.yml
Review Comment:
If it's auto-generated, it should be fine.
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861418158
##########
dev/.rat-excludes:
##########
@@ -9,6 +9,7 @@ build
.rat-excludes
LICENSE
NOTICE
+revapi.yml
Review Comment:
YAML can have comments: https://www.tutorialspoint.com/yaml/yaml_comments.htm
I think this should just add the ASF header instead of updating `.rat-excludes`.
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861428524
##########
dev/.rat-excludes:
##########
@@ -9,6 +9,7 @@ build
.rat-excludes
LICENSE
NOTICE
+revapi.yml
Review Comment:
Here's the relevant part of the diff after causing a change in the file:
```
diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml
index 03fc8d427..f310c4923 100644
--- a/.palantir/revapi.yml
+++ b/.palantir/revapi.yml
@@ -1,21 +1,85 @@
-#
-# Licensed to the Apache Software Foundation (ASF) under one
-# or more contributor license agreements. See the NOTICE file
-# distributed with this work for additional information
-# regarding copyright ownership. The ASF licenses this file
-# to you under the Apache License, Version 2.0 (the
-# "License"); you may not use this file except in compliance
-# with the License. You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing,
-# software distributed under the License is distributed on an
-# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-# KIND, either express or implied. See the License for the
-# specific language governing permissions and limitations
-# under the License.
-#
-
versionOverrides:
org.apache.iceberg:iceberg-api:release-base-0.13.0: "0.13.1"
+acceptedBreaks:
+ release-base-0.13.0:
+ org.apache.iceberg:iceberg-api:
+ - code: "java.class.defaultSerializationChanged"
+ old: "class org.apache.iceberg.PartitionSpec"
+ new: "class org.apache.iceberg.PartitionSpec"
+ justification: "Testing YAML comment on top of file"
+ - code: "java.class.defaultSerializationChanged"
+ old: "class org.apache.iceberg.Schema"
+ new: "class org.apache.iceberg.Schema"
+ justification: "Testing YAML comment on top of file"
```
--
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 #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1110359289
Another tool to consider is `me.champeau.gradle.japicmp`, which was adopted by the OpenTelemetry project in the following PR:
https://github.com/open-telemetry/opentelemetry-java/pull/3183
The rev-api plugin used in this PR is _very_ actively maintained and relatively simple, but anybody who is interested might use that for comparison.
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1146734423
Here’s an image of the tags needed. It’s just a `release-base-0.13.x` that’s needed. @KarlManong.
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r865249394
##########
build.gradle:
##########
@@ -191,6 +192,14 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ apply plugin: 'com.palantir.revapi'
+
+ revapi {
+ oldGroup = 'org.apache.iceberg'
+ oldName = 'iceberg-api'
+ oldVersion = '0.13.0'
+ }
Review Comment:
Using this seems to be not the best idea, as it would need to be updated manually across versions.
Using the gradle tasks doesn't update this.
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r865292544
##########
.palantir/revapi.yml:
##########
@@ -0,0 +1,2 @@
+versionOverrides:
Review Comment:
I set it by running `./gradlew iceberg-api:revapiVersionOverride --replacement-version 0.13.0`
--
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 diff in pull request #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r865249394
##########
build.gradle:
##########
@@ -191,6 +192,14 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ apply plugin: 'com.palantir.revapi'
+
+ revapi {
+ oldGroup = 'org.apache.iceberg'
+ oldName = 'iceberg-api'
+ oldVersion = '0.13.0'
+ }
Review Comment:
Using this seems to be not the best idea, as it would need to be updated manually across versions.
Using the gradle tasks doesn't update this, but we might consider using this for when we cut a release tag (not the release base off of master but the actual tag).
--
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 diff in pull request #4638: [WIP] API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#discussion_r861428232
##########
dev/.rat-excludes:
##########
@@ -9,6 +9,7 @@ build
.rat-excludes
LICENSE
NOTICE
+revapi.yml
Review Comment:
Unfortunately, it will have to go in the `.rat-excludes` file.
I added the header, then ran `./gradlew iceberg-api:revapiAcceptAllBreaks --justification "Testing YAML comment on top of file"` to cause an update to the file. The resulting file output had the header removed (essentially a full rewrite of the file).
So unless we want to manually re-enter the license header every time we make a chance to the file, it needs to be excluded.
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1146734308
Oh I’m sorry I missed this.
If you’re on master, can you fetch all tags from the upstream repository? `git fetch upstream`. I believe you’re missing `release-base-0.13.x` branch @KarlManong.
Commenting on closed PRs is likely to not get seen. Probably better to open an issue. But @chenjunjiedada had this issue recently and they were missing the most recent release-base branch.
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1126793562
> Thanks, @kbendick! This is great to have. I merged this. Do we expect builds to start failing now if there are binary breaking changes?
Yes. In fact, builds might start failing now (the changes to gradle check were prossibly not run in this PR’s status checks).
If you run `./gradlew check` it will run the api binary compatibility and then we can add justifications. The whole thing will tell us how.
I’ll hop on my computer and open a PR or if you make one then I’ll review it. Then from there it will be required. @rdblue
--
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 #4638: API / binary compatibility checking tool via revapi gradle plugin
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4638:
URL: https://github.com/apache/iceberg/pull/4638#issuecomment-1126791363
Thanks, @kbendick! This is great to have. I merged this. Do we expect builds to start failing now if there are binary breaking changes?
--
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