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 #689

Merged
merged 5 commits into from
Oct 17, 2024
Merged

Update dependencies #689

merged 5 commits into from
Oct 17, 2024

Conversation

odenix
Copy link
Contributor

@odenix odenix commented Oct 15, 2024

  • Update dependencies with gw updateDependencyLocks. Most updated dependencies are test dependencies.
  • Adapt code to breaking changes in library commonmark.
  • Fix test to close PackageServer exactly once (problem went unnoticed with earlier JUnit versions).

Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

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

Overall, LGTM; just not entirely sure of 0.+ dependency spec; leaving to @bioball and/or @stackoverflow to sign off.

@@ -38,15 +39,16 @@ ktfmt = "0.44"
# replaces nuValidator's log4j dependency
# something related to log4j-1.2-api is apparently broken in 2.17.2
log4j = "2.17.1"
msgpack = "0.9.0"
nexusPublishPlugin = "1.3.0"
msgpack = "0.+"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely comfortable with a 0.+, given the instability of 0 major. @bioball / @stackoverflow; thoughts?

Copy link
Contributor Author

@odenix odenix Oct 16, 2024

Choose a reason for hiding this comment

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

The difference between 0.+ and 0.9.8 is that 0.+ will resolve to the latest version when gw updateDependencyLocks is run. After that, the resolved version is locked (see lock files).

Because the Pkl build doesn’t lock Gradle plugin versions, those versions should not contain wildcards.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe @holzensp is saying that we should declare this as 0.9.+, rather than 0.+, because per semver rules, new 0.x versions can have breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I read the semver spec, all bets are off for 0.x.y versions, and 0.9.8 -> 0.9.9 may break everything.
0.9.+ risks missing that 0.10 exists. If you prefer it nevertheless, I don’t mind making this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you're right. And, looking at their release history, they have breaking changes in patch versions.

Maybe it's best to fully specify the version here; msgpack = "0.9.8".

Copy link
Contributor Author

@odenix odenix Oct 16, 2024

Choose a reason for hiding this comment

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

Personally I'm more worried about falling way behind the latest version than about the build failing to catch an incompatible change. But to move this forward, I'll change back to msgpack = "0.9.8".

By the way, I wasn't happy with the state of msgpack's Java library.
(They also never replied to my issues and PRs.)
Eventually I wrote a replacement that I'll soon turn into a public GitHub project. From the README:

MiniPack at a Glance

  • Complete implementation of the MessagePack binary serialization format.
  • Clean and modern API with JSpecify nullness annotations.
  • Designed to be correct, efficient, and extensible.
  • Embraces Java NIO: Heap and direct byte buffers, channels, channel transfers, gathering writes, buffer pooling.
  • Fast, correct, and customizable string encoding/decoding with java.nio.charset.CharsetEncoder/CharsetDecoder.
  • Optimized handling of strings used as identifiers.
  • Small JAR size (~50 KB).
  • No use of reflection or internal/unsafe JDK classes.
  • Ships as Java (JPMS) module.
  • No dependencies other than JSpecify (~3 KB).
  • Compatible with GraalVM Native Image.
  • Thoroughly benchmarked with JMH.
  • Thoroughly tested with jqwik.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Happy to entertain switching from msgpack-java to minipack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bioball ready to squash and merge from my side.

- Update dependencies by deleting lock files and regenerating them with `gw updateDependencyLocks`.
  Deleting lock files avoids strange `some.library:some.older.version=default` entries.
  Most updated dependencies are test dependencies.
- Handle breaking changes in library commonmark.
- Fix test to close PackageServer exactly once.
  This problem surfaced because JUnit 5.11 changed override rules for lifecycle methods,
  resulting in too many instead of too few close() calls.
io.leangen.geantyref:geantyref:1.3.14=default
io.leangen.geantyref:geantyref:1.3.15=compileClasspath,implementationDependenciesMetadata,runtimeClasspath,testCompileClasspath,testImplementationDependenciesMetadata,testRuntimeClasspath
com.tunnelvisionlabs:antlr4-runtime:4.9.0=pklCodegenJava,runtimeClasspath,testRuntimeClasspath
io.leangen.geantyref:geantyref:1.3.16=compileClasspath,implementationDependenciesMetadata,runtimeClasspath,testCompileClasspath,testImplementationDependenciesMetadata,testRuntimeClasspath
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like geantyref doesn't have a changelog or release notes right now; filed leangen/geantyref#31

Looking at the git diffs, though, should be fine to bump to 1.3.16.

org.jetbrains:annotations:13.0=pkldoc
org.organicdesign:Paguro:3.10.3=pkldoc
org.snakeyaml:snakeyaml-engine:2.5=pkldoc
empty=archives,default,signatures
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty weird that these ended up in the lockfile here in the first place.

Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contributions, including addressing breaking changes!

@bioball bioball merged commit d271b62 into apple:main Oct 17, 2024
5 checks passed
holzensp pushed a commit to holzensp/pkl that referenced this pull request Oct 23, 2024
- Update dependencies by deleting lock files and regenerating them with `gw updateDependencyLocks`.
  Deleting lock files avoids strange `some.library:some.older.version=default` entries.
  Most updated dependencies are test dependencies.
- Handle breaking changes in library commonmark.
- Fix test to close PackageServer exactly once.
  This problem surfaced because JUnit 5.11 changed override rules for lifecycle methods,
  resulting in too many instead of too few close() calls.
- Bump msgpack version
- Bump clikt version
- Bump Gradle plugin versions
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