Skip to content

feat(aqua): add JAR file support and ktfmt registry entry#8575

Closed
Jarvvski wants to merge 1 commit into
jdx:mainfrom
AmebaAI:ajarvis/add-ktfmt-via-aqua
Closed

feat(aqua): add JAR file support and ktfmt registry entry#8575
Jarvvski wants to merge 1 commit into
jdx:mainfrom
AmebaAI:ajarvis/add-ktfmt-via-aqua

Conversation

@Jarvvski

@Jarvvski Jarvvski commented Mar 12, 2026

Copy link
Copy Markdown

Summary

  • Add generic JAR file support to the aqua backend — when a raw format asset ends in .jar, mise now creates a shell wrapper that invokes java -jar instead of copying the JAR directly as the binary
  • Add ktfmt to the registry (aqua:facebook/ktfmt) as the first tool to use this

An example of the generated wrapper:

  #!/bin/sh
  if [ -n "$JAVA_HOME" ]; then
    exec "$JAVA_HOME/bin/java" -jar
  "/path/to/installs/ktfmt/0.53/ktfmt.jar" "$@"
  else
    exec java -jar "/path/to/installs/ktfmt/0.53/ktfmt.jar" "$@"
  fi

Context

ktfmt is distributed as a fat JAR (ktfmt-<version>-with-dependencies.jar). JAR files aren't directly executable, so the aqua backend now detects .jar assets and generates a #!/bin/sh wrapper that respects $JAVA_HOME if set.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request integrates ktfmt, a Kotlin code formatting tool, into the system's registry. This enhancement streamlines the process of maintaining consistent Kotlin code style, leveraging recent advancements in aqua's jar support and ktfmt's availability in the aqua-registry.

Highlights

  • New Tool Integration: The ktfmt Kotlin code formatter has been added to the registry, enabling standardized formatting for Kotlin projects.
  • Leveraging Existing Infrastructure: This integration utilizes the recently added jar support within aqua and ktfmt's existing entry in the aqua-registry.
Changelog
  • registry/ktfmt.toml
    • Added the configuration file for ktfmt, defining its backend, Java dependency, description, and verification tests.
Activity
  • The author, Jarvvski, created this pull request to add ktfmt to the registry.
  • This PR follows the successful implementation of jar support in aquaproj/aqua#4625.
  • It also builds on ktfmt being added to the aquaproj/aqua-registry#50235.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for the ktfmt Kotlin formatter by adding its configuration to the registry. The dependency on java is correctly specified. However, the test configuration for verifying the installation is incorrect and will likely fail. I've provided a suggestion to fix the expected output of the version check command.

Comment thread registry/ktfmt.toml Outdated
backends = ["aqua:facebook/ktfmt"]
depends = ["java"]
description = "A program that reformats Kotlin source code to comply with the common community standard for Kotlin code conventions"
test = ["ktfmt --version", "ktfmt version {{version}}"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The test configuration appears to be incorrect. The test array is typically in the format ["<command>", "<expected_output>"]. The ktfmt --version command outputs only the version number (e.g., 0.46), not a string like ktfmt version 0.46. As a result, this test is likely to fail.

To fix this, the expected output should be the version placeholder to match the actual command output.

Suggested change
test = ["ktfmt --version", "ktfmt version {{version}}"]
test = ["ktfmt --version", "{{version}}"]

@Jarvvski Jarvvski Mar 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This isn't correct:

ktfmt --version
ktfmt version 0.61

@Jarvvski Jarvvski force-pushed the ajarvis/add-ktfmt-via-aqua branch from caf03ea to 269de30 Compare March 12, 2026 20:39
@greptile-apps

greptile-apps Bot commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extends the aqua backend's format == "raw" installation path to detect .jar assets, copies the JAR alongside the install directory, and generates a #!/bin/sh wrapper that invokes java -jar <absolute-jar-path>. It also adds ktfmt as the first consumer of this new path, with correct depends = ["java"] and the upstream asset template.

Key points:

  • The Unix happy path is functionally correct: the wrapper is written to first_bin_path, the make_executable loop correctly marks the wrapper (not the JAR) executable, and the srcs/symlink_bins interaction is sound for single-binary packages.
  • The test command in registry/ktfmt.toml (ktfmt --version with expected = "ktfmt version {{version}}") is unverified — the ktfmt README does not document a --version flag, raising the possibility that the test will always fail or produce a false negative on installation checks.
  • Three items already flagged in prior review threads remain open: the hardcoded absolute JAR path in the wrapper, the lack of a Windows .cmd wrapper, and first_bin_path.with_extension("jar") silently replacing an existing extension rather than appending .jar.
  • No unit tests are added for the new code branch in aqua.rs.

Confidence Score: 2/5

  • Not safe to merge as-is — multiple open issues from prior threads remain unaddressed and the new test command is unverified.
  • The Unix installation flow is logically sound, but the PR carries three un-addressed issues flagged in earlier review rounds (hardcoded absolute path, no Windows support, incorrect use of with_extension) plus a new concern about the unverified --version test in ktfmt.toml. Any of the prior three issues can cause real user-facing failures (stale path after MISE_DATA_DIR change; broken Windows installs; silent extension clobber for future JAR tools), and the test concern means CI may not catch regressions.
  • src/backend/aqua.rs (wrapper generation logic) and registry/ktfmt.toml (unverified test command) need the most attention before merge.

Important Files Changed

Filename Overview
src/backend/aqua.rs Adds JAR detection in the format == "raw" branch: copies the JAR to first_bin_path.with_extension("jar") and writes a #!/bin/sh wrapper. Correct on Unix for the happy path; wrapper embeds an absolute JAR path (flagged separately) and has no Windows-compatible counterpart (flagged separately). The make_executable logic and srcs/symlink_bins interaction are handled correctly for the single-binary ktfmt case.
crates/aqua-registry/aqua-registry/pkgs/facebook/ktfmt/registry.yaml Adds the ktfmt aqua registry entry with format: raw and complete_windows_ext: false. Asset template correctly uses trimV to strip the leading v from the version tag. No bins/files field is needed since the fallback binary name (ktfmt) matches the expected wrapper name.
registry/ktfmt.toml Adds the mise registry entry for ktfmt with correct depends = ["java"] and aqua:facebook/ktfmt backend. The test command may be unreliable if ktfmt's JAR CLI doesn't expose --version (not documented in the ktfmt README).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[AquaBackend::install called\nformat = raw, filename = *.jar] --> B{filename.ends_with\n.jar ?}
    B -- No --> C[file::copy tarball\nto first_bin_path]
    B -- Yes --> D[Compute jar_path =\nfirst_bin_path.with_extension jar]
    D --> E[file::copy tarball\nto jar_path]
    E --> F[Generate sh wrapper\nwith absolute jar_path\nembedded via formatdoc!]
    F --> G[file::write wrapper\nto first_bin_path]
    G --> H[make_executable = true]
    C --> H
    H --> I[Loop over bin_paths:\nfile::make_executable\nwrapper script]
    I --> J{symlink_bins?}
    J -- Yes --> K[create_symlink_bin_dir:\nsymlink to wrapper in .mise-bins/]
    J -- No --> L[Done]
    K --> L
Loading

Last reviewed commit: 9d4455b

@Jarvvski Jarvvski force-pushed the ajarvis/add-ktfmt-via-aqua branch from 269de30 to f9ac009 Compare March 14, 2026 13:12
Comment thread src/backend/aqua.rs Outdated
Comment thread src/backend/aqua.rs
@Jarvvski Jarvvski force-pushed the ajarvis/add-ktfmt-via-aqua branch from f9ac009 to 737b99b Compare March 14, 2026 13:27
Comment thread src/backend/aqua.rs
@Jarvvski Jarvvski changed the title registry: add ktfmt (aqua:facebook/ktfmt) feat(aqua): add JAR file support and ktfmt registry entry Mar 14, 2026
@autofix-ci

autofix-ci Bot commented Mar 14, 2026

Copy link
Copy Markdown
Contributor

Hi! I'm autofix logoautofix.ci, a bot that automatically fixes trivial issues such as code formatting in pull requests.

I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:

  1. Allow edits by maintainers for your pull request, and then re-trigger CI (for example by pushing a new commit).
  2. Manually fix the issues identified for your pull request (see the GitHub Actions output for details on what I would like to change).

@Jarvvski Jarvvski force-pushed the ajarvis/add-ktfmt-via-aqua branch from 737b99b to 9d4455b Compare March 14, 2026 13:41
@Jarvvski

Copy link
Copy Markdown
Author

@jdx think we can look at getting this merged?

@risu729

risu729 commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

For context: Aqua recently added support in aquaproj/aqua#4625

I don't use Windows, so not too sure, but I think we shouldn't ignore them?

@risu729

risu729 commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

Also, it might be worth adding an e2e test with a pinned version of ktfmt in e2e/. The registry test will be ignored when flaky (for a week after the new release of the tool if I remember correctly).

@Jarvvski

Copy link
Copy Markdown
Author

For context: Aqua recently added support in aquaproj/aqua#4625

I don't use Windows, so not too sure, but I think we shouldn't ignore them?

I'd suggest supporting windows once others post an issue / chat in discord, instead of holding back the PR

@jdx jdx closed this Mar 19, 2026
@jdx

jdx commented Mar 19, 2026

Copy link
Copy Markdown
Owner

I don't think we should do anything like this for raw aqua types

@Jarvvski

Copy link
Copy Markdown
Author

I don't think we should do anything like this for raw aqua types

Ok, what would you prefer? Or will mise never support tools like this?

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