guava icon indicating copy to clipboard operation
guava copied to clipboard

JDiff links point to non-matching JDIFF output

Open reschke opened this issue 8 months ago • 11 comments

Guava Version

33.4.6-jre

Description

In https://github.com/google/guava/releases, the links to JDiff output is broken.

Example

The link "33.4.6-jre vs. 33.4.5-jre" links to the JDIFF "Between Guava 33.3.1-jre and Guava 33.4.6-jre".

Expected Behavior

Either fix link descriptions or link targets.

Actual Behavior

The link "33.4.6-jre vs. 33.4.5-jre" links to the JDIFF "Between Guava 33.3.1-jre and Guava 33.4.6-jre".

Packages

No response

Platforms

No response

Checklist

  • [x] I agree to follow the code of conduct.

  • [x] I can reproduce the bug with the latest version of Guava available.

reschke avatar Apr 07 '25 07:04 reschke

Thanks!

I had noticed the second half ("Between Guava 33.3.1-jre and Guava 33.4.6-jre") and convinced myself that that made some sense: A patch release wouldn't contain any API additions or removals, so we might as well diff against the most recent version that we would actually have changes relative to. I still found it little surprising.

And maybe part of the reason that I found it surprising is the inconsistency that you've pointed out, which had gone entirely over my head. Clearly we should say either the one thing or the other :)

After actually looking into this for a moment, I'm guessing that the issue is that I never updated https://github.com/google/guava/blob/87933badeccab5065b78e9c1128735ed3fe041fb/_util/util.sh#L265-L266 when I started publishing patch releases.

@cgdecker, does that sound right to you?

cpovirk avatar Apr 07 '25 12:04 cpovirk

(And this is particularly vexing right now, given that JDiff thinks that lots of APIs changed since 33.3.1, thanks to its poor understanding of type-use annotations.)

cpovirk avatar Apr 08 '25 13:04 cpovirk

I don't know if this is the right thing to do, but it looks like the comparison to the most recent version that has a lower major or minor version is intentional:

# Prints the highest non-rc release from the sorted list of releases produced
# by sort_releases. If a release argument is provided, print the highest non-rc
# release that has a major or minor version that is lower than the given
# release. For example, given "16.0.1", return "15.0". Given "16.1", return
# "16.0.1".

cgdecker avatar Apr 09 '25 19:04 cgdecker

Ah, thanks. (Who would ever have thought to read the docs? :))

I'm not sure what's best, either.

cpovirk avatar Apr 09 '25 20:04 cpovirk

I think the intention was definitely to show the diff from the last version that should actually have API differences, which generally makes sense to me (in theory there should be no meaningful JDiff at all between patch versions, though I suppose things that wouldn't directly be potentially-breaking changes like some annotation changes could be in patches). Maybe we can just change the code that generates the release notes to say what it's actually doing?

cgdecker avatar Apr 09 '25 20:04 cgdecker

Next Q:

https://guava.dev/releases/33.4.7-jre/api/diffs/changes/com.google.common.collect.TreeBasedTable.html

claims that two methods were removed, but that does not seem to be the case. What's going on?

reschke avatar Apr 10 '25 13:04 reschke

What's going on is that JDiff is unmaintained and somewhat bad at its job :) We should really replace it with japicmp. (There used to be another tool that someone used to publish reports for Guava, but I think it broke with our -jre/-android split.)

What's going on here specifically seems to be that JDiff doesn't recognize that TreeBasedTable still inherits methods like rowKeySet from StandardRowSortedTable after https://github.com/google/guava/pull/7660.

cpovirk avatar Apr 10 '25 14:04 cpovirk

Maybe we can just change the code that generates the release notes to say what it's actually doing?

That sounds reasonable.

I could even see just omitting the link to the JDiff from the release notes for patch releases. (We could even avoid generating the diffs in the first place, but omitting the link is easier.)

cpovirk avatar Apr 10 '25 18:04 cpovirk

Hi! I'd love to contribute to this feature. Comparing lists regardless of order sounds useful. I can help implement and write tests if it's still open. Let me know if you'd like me to proceed! 😊

hadiseShaaban avatar Apr 14 '25 14:04 hadiseShaaban

Hey! I will be more than happy to work on this issue as part of my first contribution to Guava. Since it's still open and unassigned, I’m happy to take it up without being formally assigned. Please let me know if there’s anything I should be aware of before I begin. Thanks!

mrunalinidudhagawali8 avatar May 03 '25 09:05 mrunalinidudhagawali8

Just following up on this PR: https://github.com/google/guava/pull/7798, and I have tested the changes locally. Let me know if there are any additional steps you'd like me to take. Happy to move this forward! Thanks!

mrunalinidudhagawali8 avatar May 14 '25 17:05 mrunalinidudhagawali8