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

Update dependencies and linting changes #264

Merged

Conversation

StefanBratanov
Copy link
Collaborator

@StefanBratanov StefanBratanov commented Feb 10, 2023

  • Update versions of most libraries and gradle plugins
  • Upgrade the gradle wrapper to 7.6
  • Update kotlin version from 1.6.10 to 1.6.21
  • Replace jmh initialization with a me.champeau.jmh gradle plugin
  • Linting changes to files based on the lintKotlin task
  • Change the jvmTarget, sourceCompatibility and targetCompatibility to Java 11 (align with the README.md saying Java 11 is needed.)
  • Small build.gradle nits suggested by IntelliJ
  • Change the two dependency badges in the README.md with one provided by cloudsmith

fixes #242

@StefanBratanov StefanBratanov marked this pull request as draft February 10, 2023 15:53
@StefanBratanov StefanBratanov marked this pull request as ready for review February 10, 2023 16:25
@StefanBratanov StefanBratanov changed the title Update versions and linting changes Update dependencies and linting changes Feb 10, 2023
@StefanBratanov StefanBratanov force-pushed the version_updates_plus_linting branch from 7e5c722 to 4af6e47 Compare February 10, 2023 21:52
@Nashatyrev
Copy link
Collaborator

All looks good to me except I would not upgrade Kotlin version unless it's strictly necessary

  • Kotlin 1.8 is still marked as 'experimental'
  • Kotlin doesn't maintain strict backward compatibility. That may also break existing lib user's codebase
  • Kotlin is still not that stable as Java.

I also had some doubts regarding usage of me.champeau.jmh gradle plugin as we had some issues with it in Teku and finally replaced it with a custom code (Consensys/teku#4817). But I tried running JMH from IDEA and from CLI and didn't noticed any problems. So we could apply this change for now and revert back in case if anything goes wrong.

@StefanBratanov
Copy link
Collaborator Author

StefanBratanov commented Feb 14, 2023

All looks good to me except I would not upgrade Kotlin version unless it's strictly necessary

  • Kotlin 1.8 is still marked as 'experimental'
  • Kotlin doesn't maintain strict backward compatibility. That may also break existing lib user's codebase
  • Kotlin is still not that stable as Java.

I also had some doubts regarding usage of me.champeau.jmh gradle plugin as we had some issues with it in Teku and finally replaced it with a custom code (Consensys/teku#4817). But I tried running JMH from IDEA and from CLI and didn't noticed any problems. So we could apply this change for now and revert back in case if anything goes wrong.

Thanks Anton for the review. What is latest version of Kotlin which is not experimental or just reverting back to 1.6 makes more sense?

@Nashatyrev
Copy link
Collaborator

What is latest version of Kotlin which is not experimental or just reverting back to 1.6 makes more sense?

I would leave 1.6 unless there is a need for upgrade

@StefanBratanov
Copy link
Collaborator Author

What is latest version of Kotlin which is not experimental or just reverting back to 1.6 makes more sense?

I would leave 1.6 unless there is a need for upgrade

Cool. Makes sense. I changed it to the latest version of 1.6. Had to downgrade couple of libraries to previous versions.

@@ -40,7 +40,7 @@ dependencies {
api("io.netty:netty-all:4.1.87.Final")
api("com.google.protobuf:protobuf-java:3.21.12")

implementation(kotlin("stdlib"))
implementation(kotlin("stdlib-jdk8"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe no need to revert this lib back to stdlib-jdk8 as we switched to Java-11 target

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I read from Kotlin 1.8 stdlib-jdk8 is merged into kotlin-stdlib . Otherwise, it is preferred to use stdlib-jdk8 to make use of the jdk8 extensions.

@Nashatyrev
Copy link
Collaborator

Had to downgrade couple of libraries to previous versions.

Yep, that's the downside of not upgrade. Seems like no critical libs has to be downgraded though

@StefanBratanov StefanBratanov force-pushed the version_updates_plus_linting branch from fcf0a47 to 92c5652 Compare February 14, 2023 16:27
@StefanBratanov
Copy link
Collaborator Author

Just force pushed a cleaner commit without a lot of linting changes (which applies only to the later versions of Kotlin and the kotlinter)

@Nashatyrev Nashatyrev merged commit fe2a1bc into libp2p:develop Feb 15, 2023
@StefanBratanov StefanBratanov deleted the version_updates_plus_linting branch February 15, 2023 09:09
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.

Netty 4.1.69 don't compile on android
2 participants