[jaykay12]

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

Description

Flipping CLI default flag to PicoCLI

Based upon the Dev-List item: https://lists.apache.org/thread.html/zxckzh6v98xqjfxf8kn3l73g3593nfly

Solution

SOLR_PICOCLI=false is the hatch, which helps us be backward-compatible & use commons-cli in case any unforeseen error comes.

Tests

NA

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

[epugh]

One suggested change, but otherwise LGTM!

[epugh]

This should be removed on Solr 11 Ref Guide, of course, we need it on solr 10. I hope we see commons-cli removed from Solr 11 very soon!

[jaykay12]

yes, we will plan for the cleanup as well real quick.

[nick-boss-tech]

🤖 AI text below 🤖 (reviewed by Nick Shanin)

Thanks for this. SOLR_PICOCLI=false is the escape hatch as described. Two things to tidy before merge:

1. The changelog file is still named changelog/unreleased/SOLR-17697-migrate-cli-to-picocli.yml. This PR is SOLR-18561, so the file name should match.

2. The PR body has "I have run ./gradlew check" and "I have added tests" unchecked. Because the default flips for every tool, a bats case for the fallback would make it explicit. For example, in solr/packaging/test/test_help.bats:

@test "create help with SOLR_PICOCLI=false uses the commons-cli fallback" {
  SOLR_PICOCLI=false run solr create --help
  assert_success
  assert_output --partial 'bin/solr create'
  refute_output --partial 'Usage: solr create'
}

I haven't run this example, so the expected strings are unconfirmed.

CI failure: The "Run Solr Script Tests" job fails on one assertion in test_ssl.bats (test 88, line 103). The PR can't merge while that job fails. The fix is PR #5193 for SOLR-18565, a one-line change. PR #5193 is now merged into main, so updating this branch from main should clear the check.

[jaykay12]

self-review ✅

[jaykay12]

The changelog file is still named changelog/unreleased/SOLR-17697-migrate-cli-to-picocli.yml. This PR is SOLR-18561, so the file name should match.

actually, we are maintaining 1 single CL file for the picoCLI migration & not every PR wise. thus, the cl file is updated & the jira it carries is the main parent one, under which all these subtasks land upon.

CI failure: The "Run Solr Script Tests" job fails on one assertion in test_ssl.bats (test 88, line 103). The PR can't merge while that job fails. The fix is #5193 for SOLR-18565, a one-line change. Once that is on main, updating this branch from main should clear the check.

thanks @nick-boss-tech for the review.
ummm, if that is the case, bats test should have been failing for every other PR right? because bats failed for the suite which runs the native commons-cli based CLI, not the picoCLI ones. the former remains untouched with this PR.

that makes me doubt, if this is intermittent only & we should check it again.

@epugh requesting you to retrigger the workflows again, & we will keep an eye on the bats this time.

[jaykay12]

there are 4 bats that requires fix, I will be raising a follow-up PR for the same, for now added a Todo here & adding the remaining bats.

this takes us from 41 -> 92 assertions now.

[janhoy]

So this means we now run the entire BATS suite for picocli, right?

[jaykay12]

we are running 26/30 bats suite for now, the 4 that are not in the list: tests_basic_auth, test_help, test_post & test_stream. I will work on debugging & fixing in a quick follow-up PR & then those will also get added. we will then remove this Todo & then we can say all 30 BATs suite runs for PicoCLI.

[jaykay12]

@janhoy Here it is: #5204

[janhoy]

I think this NOTE shuold be replaced with a short note about commons-cli being available as an escape hatch. The link to xref:cli/index.adoc[] should instead be a new paragraph == Man pages or similar, telling that each command has its own documentation page and the link.

I wonder if perhpas most of the remainder of this adoc page can be deleted since it is manually maintained and largely redundant with the new man pages? There are some general sections near the bottom that could perhaps stay.

[jaykay12]

I think this NOTE shuold be replaced with a short note about commons-cli being available as an escape hatch. The link to xref:cli/index.adoc[] should instead be a new paragraph == Man pages or similar, telling that each command has its own documentation page and the link.

have done few changes in the wordings here.

[jaykay12]

I wonder if perhpas most of the remainder of this adoc page can be deleted since it is manually maintained and largely redundant with the new man pages? There are some general sections near the bottom that could perhaps stay.

imo, we should not do this cleanup now. This solr-control-script-reference.adoc ties back to commons-cli & till the time we are supporting commons-cli there is no harm in keeping it. when we remove commons-cli support, this ref guide also dies alongside it.

we are explicitly asking users to report any behaviour diff if they observe with respect to this page over in the bottom of index.sdoc.

[janhoy]

The wording "See xref:cli/index.adoc[] if you would like to try it" feels strange when this is now the actual default. So we should instead actively point people to the new CLI page as the reference docs for what is now the default CLI commands.

IMO these two pages shuold be written as if picocli was our only choice, and mention commons-cli as a side-note and escape hatch if they run in to a bug.

It's probably ok to keep some of the tool descriptions on the control-script-reference page, unless they contradict options or output from picocli. Perhaps we should add a heading saying that the below section describes the fallback commons-cli and may differ from what is now default?

[jaykay12]

have done few changes, pls have a look now, does it look better?

[janhoy]

I'm still sceptical to keeping such a large CLI usage block this prominent when it does not apply (fully) to the default CLI that 99% will be using. Could we move the section to a new sub page "Commons-CLI command reference"? That page could describe the escape hatch and document the old behavior.

[janhoy]

DId a review, sorry for not grouping all comments under this review. I did not yet take it for a spin or render the ref guide to inspect how it reads. After next round of edits I'll take it for a spin.

PS: I merged Nick's small ssl bats change but did not merge it into this branch so won't see the effect yet.

[jaykay12]

@janhoy @epugh PR is ready for next round of review. ✨

[janhoy]

I did not have time to spin it up right now, but some ad-hoc comments on the docs. @epugh please chime in if you have opinions on the docs and how we structure them now that PicoCLI becomes default.

[janhoy]

An end user won't know what a "source annotation" is. The ref guide is for end users, we should describe what they see, the commands they write and the response they get back, not code-level things.

[janhoy]

It's dangerous to use a term like legacy, in this context we could perhaps say that before Solr 10.2 -v was a short-hand for value for the config command, but not any more.

The legacy comment is general for any other place it may have been used.

Also, I'm not sure that "migration changes" even belong in this doc page. It will duplicate the upgrade notes in major-changes-in-solr-10, section 10.2. so I'd prefer if we just do a WARNING callout on this page with a link to that upgrade notes section, and document all deliberate breaking changes for PicoCLI there only.

[janhoy]

I'm still sceptical to keeping such a large CLI usage block this prominent when it does not apply (fully) to the default CLI that 99% will be using. Could we move the section to a new sub page "Commons-CLI command reference"? That page could describe the escape hatch and document the old behavior.

[nick-boss-tech]

Review TL/DR:

• P2: PicoCLI tests can run Commons CLI instead. The task no longer explicitly sets SOLR_PICOCLI=true. An inherited false makes both tasks test Commons CLI. Set both task modes explicitly. Location
• P3: Upgrade notes contradict the change. They still say the default CLI is unchanged and refer to snapshot spellings whose explanation was deleted. Location
  Connection to SOLR-18566: Fixing BATs for PicoCLI migration #5204: its presence-based assertion helper conflicts with SOLR-18561 | Flipping CLI default flag to PicoCLI #5190’s task configuration. Explicit false selects PicoCLI expectations; unset selects Commons CLI expectations despite the new default. The two PRs need consistent explicit mode selection.

Review Details:
🤖 AI text below and above 🤖 (reviewed by Nick Shanin)

@jaykay12 Thanks for clarifying the shared migration changelog in the earlier discussion. Keeping the entry under SOLR-17697 makes sense, and I confirmed that the SSL BATS fix from #5193 is already included in this branch.

Reviewing f76e0589ef06 alongside #5204, I found two additional issues:

1. integrationTestsPicocli can silently exercise Commons CLI. In configureBatsTask, the updated code sets SOLR_PICOCLI=false for the Commons CLI task but leaves the PicoCLI task’s value inherited. If the invoking environment already exports SOLR_PICOCLI=false, that value reaches the Java selector, so the task named integrationTestsPicocli also runs Commons CLI. Previously, its explicit true setting prevented this.

   Could both tasks set their intended value explicitly, for example usePicocli ? 'true' : 'false'? That would make their coverage independent of the invoking environment. A separate smoke test with the variable unset could verify that PicoCLI is actually the product default.

   This also connects to SOLR-18566: Fixing BATs for PicoCLI migration #5204: its new assert_output_for_cli helper treats any nonempty value, including false, as PicoCLI. After combining these changes, the Commons CLI task would select PicoCLI expectations. Conversely, an unset value in the PicoCLI task would select Commons CLI expectations. Explicit task values and matching boolean checks in the helper should address both sides. The restart-help skip in SOLR-18566: Fixing BATs for PicoCLI migration #5204 needs the same treatment.

2. The 10.2 upgrade notes retain contradictory text. The new PicoCLI subsection still says “The default command line interface is not changed.” It also refers to “The old spellings” after removing the snapshot-command explanation and replacement examples. Could the snapshot migration remain a separate subsection, with the default-switch explanation alongside it? This would also give the compatibility changes a clear home if they are moved here as Jan suggested.

For verification, I would check that each named task selects its intended parser even when the invoking environment specifies the opposite value, followed by the relevant help/post coverage from #5204.

These findings are from static inspection; I have not run Gradle or BATS.