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

Revert "Upgraded to JUnit5 (#536)" #694

Merged
merged 2 commits into from
Jul 1, 2022
Merged

Revert "Upgraded to JUnit5 (#536)" #694

merged 2 commits into from
Jul 1, 2022

Conversation

wetneb
Copy link
Member

@wetneb wetneb commented Jun 16, 2022

Upgrading to JUnit5 requires upgrading OkHTTP to a newer version, but our okhttp-signpost dependency still relies on an earlier version. This means that at the moment we depend on two different versions of the same library, causing #600.

This closes #600.

This reverts commit 522abd4.

This is a temporary workaround. We should then consider:

  • whether we can upgrade okhttp-signpost to OkHTTP 5 (the maintainer looks responsive)
  • or whether there are alternatives to okhttp-signpost which use more recent versions of OkHTTP

Also, I propose we only migrate to OkHTTP 5 once we have a stable release of that library (so that we do not depend on an alpha version as currently).

Upgrading to JUnit5 requires upgrading OkHTTP to a newer version, but our okhttp-signpost
dependency still relies on an earlier version.

This closes #600.

This reverts commit 522abd4.
@robertvazan
Copy link
Collaborator

I don't quite remember the changes I have made in #536, but pull request comment indicates that if early upgrade to okhttp 5 causes trouble, replacing mockwebserver would be an option. I had some plans in this direction. I had to stop working on WDTK for lack of time, but perhaps I could take a quick look at this.

Making lots of edits and then reverting them wastes a lot of effort. It also propagates the problem from broken to healthy components (from okhttp to junit in this case) instead of fixing the problem at the source.

BTW, I am not aware of WDTK referencing multiple versions of okhttp as you say in #600. Can you show me where this is done?

@wetneb
Copy link
Member Author

wetneb commented Jun 16, 2022

Sure, I do not particularly enjoy reverting this commit and solving all the associated merge conflicts. If you have other ways to solve this issue I am happy to hear them.

You can see that we rely on multiple version of okhttp by running mvn dependency:tree on the master branch. For the wdtk-wikibaseapi module it gives you this:

[INFO] org.wikidata.wdtk:wdtk-wikibaseapi:jar:0.13.4-SNAPSHOT
[INFO] +- org.wikidata.wdtk:wdtk-datamodel:jar:0.13.4-SNAPSHOT:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-annotations:jar:2.13.2:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-core:jar:2.13.2:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-databind:jar:2.13.2:compile
[INFO] |  +- com.fasterxml.jackson.datatype:jackson-datatype-jdk8:jar:2.13.2:compile
[INFO] |  \- org.threeten:threeten-extra:jar:1.7.0:compile
[INFO] +- org.wikidata.wdtk:wdtk-util:jar:0.13.4-SNAPSHOT:compile
[INFO] |  \- org.apache.commons:commons-compress:jar:1.21:compile
[INFO] +- org.wikidata.wdtk:wdtk-testing:jar:0.13.4-SNAPSHOT:test
[INFO] +- com.squareup.okhttp3:okhttp-urlconnection:jar:5.0.0-alpha.7:compile
[INFO] |  +- com.squareup.okhttp3:okhttp-jvm:jar:5.0.0-alpha.7:compile
[INFO] |  |  +- com.squareup.okio:okio-jvm:jar:3.1.0:compile
[INFO] |  |  +- org.jetbrains.kotlin:kotlin-stdlib:jar:1.6.21:compile
[INFO] |  |  |  \- org.jetbrains:annotations:jar:13.0:compile
[INFO] |  |  \- org.jetbrains.kotlin:kotlin-stdlib-common:jar:1.6.21:compile
[INFO] |  \- org.jetbrains.kotlin:kotlin-stdlib-jdk8:jar:1.6.21:compile
[INFO] |     \- org.jetbrains.kotlin:kotlin-stdlib-jdk7:jar:1.6.21:compile
[INFO] +- com.squareup.okhttp3:mockwebserver3-junit5:jar:5.0.0-alpha.7:test
[INFO] |  +- com.squareup.okhttp3:mockwebserver3:jar:5.0.0-alpha.7:test
[INFO] |  \- org.junit.jupiter:junit-jupiter-api:jar:5.8.2:test
[INFO] |     +- org.opentest4j:opentest4j:jar:1.2.0:test
[INFO] |     +- org.junit.platform:junit-platform-commons:jar:1.8.2:test
[INFO] |     \- org.apiguardian:apiguardian-api:jar:1.1.2:test
[INFO] +- se.akerfeldt:okhttp-signpost:jar:1.1.0:compile
[INFO] |  +- com.squareup.okhttp3:okhttp:jar:3.0.0-RC1:compile
[INFO] |  |  \- com.squareup.okio:okio:jar:1.6.0:compile
[INFO] |  \- oauth.signpost:signpost-core:jar:1.2.1.2:compile
[INFO] |     \- commons-codec:commons-codec:jar:1.3:compile
[INFO] +- org.junit.jupiter:junit-jupiter:jar:5.8.2:test
[INFO] |  +- org.junit.jupiter:junit-jupiter-params:jar:5.8.2:test
[INFO] |  \- org.junit.jupiter:junit-jupiter-engine:jar:5.8.2:test
[INFO] |     \- org.junit.platform:junit-platform-engine:jar:1.8.2:test
[INFO] +- org.hamcrest:hamcrest-core:jar:2.2:test
[INFO] |  \- org.hamcrest:hamcrest:jar:2.2:test
[INFO] +- org.mockito:mockito-core:jar:4.5.1:test
[INFO] |  +- net.bytebuddy:byte-buddy:jar:1.12.9:test
[INFO] |  +- net.bytebuddy:byte-buddy-agent:jar:1.12.9:test
[INFO] |  \- org.objenesis:objenesis:jar:3.2:test
[INFO] +- org.slf4j:slf4j-api:jar:1.7.36:compile
[INFO] \- org.apache.commons:commons-lang3:jar:3.12.0:compile

In this output, notice that we have both com.squareup.okhttp3:okhttp:jar:3.0.0-RC1 and com.squareup.okhttp3:okhttp-jvm:jar:5.0.0-alpha.7, which is the source of the problem in my understanding.

@robertvazan
Copy link
Collaborator

I see. The bug was actually introduced by later okhttp upgrades. Since alpha 4, there are two artifacts: okhttp and okhttp-jvm. WDTK now depends on okhttp-jvm 5, which allows maven to include okhttp 3 as well. This will be a problem in the future anyway, regardless of whether okhttp 5 is adopted first by WDTK or by dependent projects. Merely downgrading to okhttp 3 will not solve the problem. I am starting to think that using okhttp in a library requires bundling it under renamed package, but this can create new problems. I should probably create issue in okhttp asking for recommended solution.

@wetneb
Copy link
Member Author

wetneb commented Jun 17, 2022

I do not think bundling it under a renamed package would make much difference in this case since that is not a conflict between our version of OkHTTP and our users', it is our own dependencies that are inconsistent…
For me the clear solution is to migrate okhttp-signpost to a newer version of okhttp, but I think we should do this migration towards a stable version of okhttp, not an alpha one (and wdtk should not depend on an alpha release of okhttp itself).

@robertvazan
Copy link
Collaborator

I wouldn't worry about okhttp-signpost. Okhttp is still backward compatible, so okhttp-signpost will work with okhttp 5. Merely using maven's exclusions to prevent okhttp-signpost's version of okhttp from propagating into WDTK's dependencies is enough to solve the current problem. If okhttp-signpost is your only concern, then exclusions is a cheap and effective solution.

The problem will however come back when some dependent project uses okhttp 3/4. Or conversely, if WDTK downgrades, dependent projects using okhttp 5 will run into the same problem. I have asked okhttp devs about this: square/okhttp#7339

I nevertheless share the sentiment of avoiding alpha dependencies when stable version is available. I will look into mockwebserver alternatives.

@wetneb
Copy link
Member Author

wetneb commented Jul 1, 2022

Merging for the sake of having a release where this problem is fixed. Very happy for this merge to be reverted whenever we have a better solution to this problem.

@wetneb wetneb merged commit 7dcf4da into master Jul 1, 2022
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.

NoSuchMethodError when following FetchOnlineDataExample
2 participants