SOLR-17697: Implement picocli for package command / PackageTool by jaykay12 · Pull Request #4739 · apache/solr

jaykay12 · 2026-08-15T10:45:18Z

https://issues.apache.org/jira/browse/SOLR-17697

Description

Implementing PicoCLI for the PackageTool

Solution

Used Prompt provided in the Jira & got initial migration done by Cursor.

Majoryly, common code which was being used in the commons-cli call path, which could be used in picocli as well, is being refactored to private functions.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide
I have added a changelog entry for my change

jaykay12 · 2026-08-15T13:18:45Z

This PR supports the PicoCLI migration: #3254

@epugh @janhoy please review once u have time.

Copilot

Pull request overview

Migrates PackageTool to picocli while retaining the legacy Commons CLI path.

Changes:

Adds picocli options, execution, and connection resolution.
Runs existing package tests through both CLI paths.
Registers and documents the package command.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`solr/core/src/java/org/apache/solr/cli/PackageTool.java`	Implements picocli support.
`solr/core/src/java/org/apache/solr/cli/SolrCLI.java`	Registers the package subcommand.
`solr/core/src/test/org/apache/solr/cli/PackageToolTest.java`	Generalizes tests across invocation paths.
`solr/core/src/test/org/apache/solr/cli/PackageToolPicocliTest.java`	Exercises the picocli path.
`solr/solr-ref-guide/modules/deployment-guide/pages/cli/solr-package.adoc`	Adds generated CLI documentation.
`solr/solr-ref-guide/modules/deployment-guide/deployment-nav.adoc`	Adds package documentation navigation.

Suppressed comments (1)

solr/core/src/java/org/apache/solr/cli/PackageTool.java:164

This drops the legacy -p spelling for package parameters. The existing package usage text still advertises -p (line 423), and the test comment identifies it as a deprecated value that must continue to work; picocli will reject it unless it is declared explicitly.

      names = {"--param"},

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

janhoy · 2026-08-22T09:50:54Z

+
+[%hardbreaks]
+  # Add a package repository
+  bin/solr package add-repo myrepo https://my.repo.example/bin/solr packages


Good catch, and the root cause is in the doc generator rather than here: postProcessCliManPage in solr/solr-ref-guide/build.gradle (~line 698) does content.replaceAll("${baseName}(?!-)", newTitle), and the (?!-) lookahead doesn't prevent solr-package from matching inside solr-packages. package is the first migrated command whose name is a prefix of a plausible word, so this will bite future tools too.

Two separate fixes:

In this PR: change the example URL in the @Command(footer = ...) to something that doesn't embed the command stem (e.g. https://my.repo.example/repo) and re-run ./gradlew :solr:solr-ref-guide:generateCliDocs.

Follow-up on the feature branch: tighten the regex to a real word boundary ((?![-\w]) rather than (?!-)), since that change affects every generated CLI page.

fixed.

sure, i will keep a tab & raise follow-up PR for the same.

jaykay12 · 2026-08-22T22:05:58Z

+      String collection,
+      boolean noPrompt) {}
+
+  // --- picocli fields ---


done.

Regarding line 164, couldn't understand.

No, I'm not so familiar with existing CLI syntax for package commands, copilot was worried that an existing -p option that works in commons-cli won't work with picocli? Have not dug in

janhoy

This is a complex tool, congratulations on picking it 😉

Main comment is that we should use Picoclis sub commands to drive the command param/options parsing, help output and auto-generated docs. It's not a goal to preserve 1:1 help output from commons-cli.

I used Claude Code to help me find discrepancies and author these review comments, so bear over with it being detailed some places.

janhoy · 2026-08-22T10:05:01Z

+      paramLabel = "COMMAND",
+      description =
+          "Package command: add-repo, add-key, list-installed, list-available, list-deployed, install, deploy, undeploy, uninstall.")
+  private String cmd;


Model the nine package commands as real picocli subcommands.

Right now cmd is a plain String positional and everything after it lands in the ARGS catch-all below, with dispatch done by a hand-written switch. The branch already has the pattern we want in ZkTool, which declares subcommands = {ConfigSetDownloadTool.class, ZkCpTool.class, ...} and gets synopsis, arity checking and per-command help for free.

Concretely, what we lose by not doing that here:

bin/solr package install --help can't produce install-specific help, and the generated ref-guide page has an empty == Commands section (see my comment on solr-package.adoc).

No arity validation. bin/solr package add-repo myrepo throws ArrayIndexOutOfBoundsException on cmdArgs[1] instead of picocli's "Missing required parameter".

Every option is advertised globally even though each belongs to one or two commands: --collections / --cluster / --param / --update are deploy/undeploy-only, and -c/--collection is list-deployed-only. Nothing tells the user, and nothing rejects a wrong combination.

The printRed("Either specify --cluster ... or --collections ...") runtime checks in deploy/undeploy are exactly what @ArgGroup(exclusive = true, multiplicity = "1") on a deploy subcommand expresses declaratively.

An unknown command becomes RuntimeException("Unrecognized command: ...") rather than picocli's standard unknown-subcommand usage error.

Suggestion: @Command(name = "package", subcommands = {AddRepo.class, AddKey.class, ListInstalled.class, ListAvailable.class, ListDeployed.class, Install.class, Deploy.class, Undeploy.class, Uninstall.class}), with each nested command carrying only its own @Parameters/@Option. The rest of my comments assume this lands.

PS: We should probably extend the LLM prompt template to identify this pattern of sub-commands and guide it to rework in native PicoCli fashion

janhoy · 2026-08-22T10:05:01Z

+// end::picocli-generated-man-section-arguments[]
+
+// tag::picocli-generated-man-section-commands[]
+// end::picocli-generated-man-section-commands[]


The == Commands section is generated empty, because package exposes its nine commands as a free-text positional rather than as picocli subcommands (see my comment on the COMMAND positional in PackageTool.java).

That means the ref-guide page documents the entire package CLI as one option list plus a single prose sentence listing command names — no per-command synopsis, no indication of which options apply to which command. Once the commands become real subcommands, this section populates itself and each command also gets its own generated page and nav entry, matching how zk is documented today.

janhoy · 2026-08-22T10:05:01Z

+    return 0;
+  }
+
+  private String resolveSolrUrl(String credentials) throws Exception {


These two resolvers duplicate connection logic that already exists in three places.

resolveZkHost below re-implements ZkConnectionOptions.resolveZkHost() almost verbatim — same StatusTool.reportStatus → cloud → ZooKeeper lookup, same (embedded) suffix stripping — and resolveSolrUrl copies the stderr string from CLIUtils.normalizeSolrUrl(CommandLine) literally. CreateTool.callTool() and DeleteTool.callTool() already carry the same copy/paste, so this would make it the third and fourth.

Suggestion: hoist resolveSolrUrl(credentials) and resolveZkHost(credentials) onto ConnectionOptions (it already has effectiveSolrUrl() / effectiveZkHost()) and have PackageTool, CreateTool and DeleteTool all call them. With subcommands this becomes even more valuable: resolution belongs on the parent package command once, inherited by all nine.

Minor while you're in here: cloud.get("ZooKeeper").toString() NPEs if the key is absent; ZkConnectionOptions casts and null-checks instead.

janhoy · 2026-08-22T10:05:02Z

+    }
+    String defaultUrl = CLIUtils.getDefaultSolrUrl();
+    CLIO.err(
+        "Neither --solr-connection, --zk-host or --solr-url parameters, nor SOLR_CONNECTION, ZK_HOST env var provided, so assuming solr url is "


This message promises more than the code delivers: the only fallback actually consulted is EnvUtils.getProperty("zkHost") (line above, and again in resolveZkHost). SOLR_CONNECTION / the solrConnection property is never read, so it works under commons-cli (via CLIUtils.resolveSolrConnectionFromCli) but is silently ignored on the picocli path — while this message claims it was checked.

Worth noting the underlying cause is a framework gap rather than something to solve per-tool: picocli doesn't apply the CliDefaultValueProvider to @ArgGroup members when the group is unmatched, which is exactly why connectionOptions can be null here. That's the still-unchecked "Solve value-fallback to ENV" milestone on #3254.

If you spin this into a separate PR, then add a TODO or NOCOMMIT comment with a link so we know there is a bug here that depends on some other PR.

janhoy · 2026-08-22T10:05:02Z

+    PackageFlags packageFlags =
+        new PackageFlags(collections, cluster, param, update, collection, noPrompt);
+    executePackage(solrUrl, zkHost, credentials, cmd, args, packageFlags);
+    return 0;


callTool() returns 0 unconditionally, so the exit codes advertised in @Command(exitCodeList = ...) never actually occur: an install that prints printRed(pkg + " installation failed.") still exits 0, as does a deploy rejected for missing --cluster/--collections. Only a thrown exception yields 1, via ToolBase.call.

That makes bin/solr package install foo; echo $? misleading in scripts. Either return 1 on those failure paths (per subcommand, once they're split out) or drop the exit-code-1 entry from the annotation.

janhoy · 2026-08-22T10:05:02Z

+
+/**
+ * Runs all {@link PackageToolTest} tests through the picocli invocation path.
+ */


./gradlew tidy hasn't been run — ./gradlew :solr:core:spotlessJavaCheck currently fails on this file and on PackageToolTest.java, so ./gradlew check will fail too.

janhoy · 2026-08-22T10:05:02Z

          packageManager.close();
        }
      }
      log.info("Finished: {}", cmd);


This logs the cmd field, which is only populated on the picocli path, rather than the command parameter that executePackage was given. On the commons-cli path it prints Finished: null. Should be command.

janhoy · 2026-08-22T10:05:02Z

              + "don't print stack traces, hence special treatment is needed here."
              + "Need to turn off logging, and SLF4J doesn't seem to provide for a way.")
  public void runImpl(CommandLine cli) throws Exception {
+    String solrUrl = CLIUtils.normalizeSolrUrl(cli);


Hoisting the connection resolution into runImpl moved it outside the Configurator.setRootLevel(Level.OFF) window that starts in executePackage. Both normalizeSolrUrl and getZkHost can hit ZooKeeper/Solr and log, so the "logging free, clean output going through to the user" intent no longer holds for that phase. callTool() has the same shape.

Either move the level switch so it also wraps resolution, or drop the comment if the narrower window is deliberate.

janhoy · 2026-08-22T10:05:02Z

+            packageManager.undeploy(packageName, collections, packageFlags.cluster());
+          } else {
+            printRed(
+                "Either specify --cluster to undeploy cluster level plugins or -collections <list-of-collections> to undeploy collection level plugins");


Pre-existing typo now being carried over: -collections should be --collections (the deploy counterpart a few lines up already says --collections). Cheap to fix while this line is being touched — and if this check becomes an @ArgGroup on an Undeploy subcommand, the message goes away entirely.

janhoy · 2026-08-22T10:05:02Z

          .desc("Don't prompt for input; accept all default choices, defaults to false.")
          .get();

+  record PackageFlags(


Worth a short javadoc, matching AuthTool.AuthParams ("Parameters for the auth sub-commands, independent of the command line parser") — that record also documents its non-obvious field. Keeping the pattern self-documenting matters as more tools copy it.

That said, once the nine commands are separate subcommands this record likely shrinks a lot or splits per command, since most fields apply to only one or two of them — so maybe settle its final shape first.

jaykay12 added 3 commits August 15, 2026 13:45

prompt done

f71a932

minor

f61cdeb

tidying done

86913e5

github-actions Bot added the cat:cli label Aug 15, 2026

generated cli docs

124256c

github-actions Bot added the documentation Improvements or additions to documentation label Aug 15, 2026

epugh mentioned this pull request Aug 15, 2026

Refactor CLI Tools to be independent of commons-cli #4695

Open

tests

4e2e528

github-actions Bot added the tests label Aug 15, 2026

janhoy requested a balanced review from Copilot August 22, 2026 09:21

Copilot started reviewing on behalf of janhoy August 22, 2026 09:21 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

janhoy requested changes Aug 22, 2026

View reviewed changes

jaykay12 added 2 commits August 23, 2026 03:00

fixed url

e9cefa8

help added

5e76d92

Uh oh!

Conversation

jaykay12 commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

jaykay12 commented Aug 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janhoy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaykay12 commented Aug 15, 2026 •

edited

Loading