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

Return builder from setHostnameVerificationPolicy #588

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

osipxd
Copy link
Contributor

@osipxd osipxd commented Oct 11, 2024

Currently, it is impossible to add setHostnameVerificationPolicy to a call chain

@ok2c
Copy link
Member

ok2c commented Oct 11, 2024

@osipxd Damn. This is an API breaking change. We will have to deprecate this method and add another one with a different name. What is even more worrisome is the API breakage has not been detected by the build.

@ok2c
Copy link
Member

ok2c commented Oct 12, 2024

@osipxd I suppose I was wrong. The return type is not a part of method name signature and changing return type from void to something else does not cause compile incompatibility. japicmp apparently handles the case correctly.

@ok2c ok2c merged commit f6889c5 into apache:master Oct 12, 2024
10 checks passed
@ok2c
Copy link
Member

ok2c commented Oct 12, 2024

Cherry-picked to 5.4.x

@osipxd osipxd deleted the patch-1 branch October 12, 2024 11:23
@garydgregory
Copy link
Member

I tried changing the return type on a static method in a different project (Apache Commons IO) and JApiCmp failed with:

[ERROR] Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.23.0:cmp (default-cli) on project commons-io: There is at least one incompatibility: org.apache.commons.io.FileUtils.touch(java.io.File):METHOD_RETURN_TYPE_CHANGED -> [Help 1]

We are not using the latest version in HC parent, so this could be a bug in an old version of JApiCmp.

The JLS says that changing a return type is the same as removing and adding a method.
See 13.4.15. Method Result Type

@ok2c
Copy link
Member

ok2c commented Oct 12, 2024

@garydgregory

  1. We are using version 0.21.2 of JApiCmp which almost the latest.
  2. The change-set does not change the return type. It changes from void (no return type) to a return type. Logically this should compile as before.

@garydgregory
Copy link
Member

I'm still skeptical, this looks to me like 2 bugs in JApiCmp 😞

  • The change from non-final to final breaks subclasses since the containing class is public and non-final.
  • The return type change

After I run:

mvn -DskipTests -pl httpclient5 -Dhc.japicmp.version=0.23.0

JApiCmp thinks the method is "NEW" and "METHOD_ADDED_TO_PUBLIC_CLASS". Instead, I think it should report "METHOD_RETURN_TYPE_CHANGED". It should also note the change from non-final to final but it doesn't since it considers the method new, which according to the JLS it is.

There's been a few bugs in this area: https://github.com/siom79/japicmp/issues?q=is%3Aissue++return+

@ok2c
Copy link
Member

ok2c commented Oct 12, 2024

@garydgregory Do you want me to revert the commit and ask @osipxd to re-spin the PR with a more conservative fix?

@garydgregory
Copy link
Member

Hi @ok2c
Tl;dr: Yes.
I think we need to decide whether we want 100% binary compatibility or not. If it were my decision alone I would strive for 100% BC. Jar hell is too painful.

@ok2c
Copy link
Member

ok2c commented Oct 13, 2024

@garydgregory @osipxd Commit reverted and replaced by 001eff7

Please take a look

@garydgregory
Copy link
Member

LGTM, TY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants