gnodet commented on PR #13208:
URL: https://github.com/apache/maven/pull/13208#issuecomment-6060241844
Good idea — moving `OutputCapabilities` into the Maven 4 API layer would
make it a proper first-class service accessible via
`session.getService(OutputCapabilities.class)`.
A few things to think through before committing to this direction:
**What moves where**
- `OutputCapabilities` interface + `Destination` enum →
`api/maven-api-core`, package `org.apache.maven.api.services`, implements
`Service` (required for `Session.getService()`)
- `DefaultOutputCapabilities` (the mutable singleton with
`install()`/cleanup) → `impl/maven-impl`, package `org.apache.maven.impl`,
annotated `@Named @Singleton @Priority(-1)` — this is the fallback that returns
`UNKNOWN` in embedded contexts
- `TerminalOutputCapabilities` (JLine probe logic) → stays in
`impl/maven-cli` — it depends on `FastTerminal`, `TerminalExt`,
`ExecTerminalProvider`, which are JLine internals; moving them closer to the
API would leak JLine into `maven-impl`
- A higher-priority CLI override in `impl/maven-cli` or `impl/maven-jline`
wires the actual terminal probe into the service — same pattern as
`DefaultMessageBuilderFactory` (`@Priority(-1)` in `maven-impl`) overridden by
`JLineMessageBuilderFactory` (`@Priority(10)` in `maven-jline`)
**The `install()`/cleanup problem**
The current design has `DefaultOutputCapabilities.install()` return an
`AutoCloseable` that is added to `LookupContext.closeables` — this is how it
resets to `UNKNOWN` between resident invocations. If
`DefaultOutputCapabilities` lives in `maven-impl` and is a pure `Service`, the
install/cleanup contract becomes internal CLI plumbing — the API interface
exposes only `getDestination()` / `getEncoding()`, and the mutation lives in a
`DefaultOutputCapabilities` cast. That's the same pattern the current PR uses;
it just changes which module owns the default impl.
Alternatively: ditch the `install()`/resident cleanup entirely and make the
service read from a thread-local or a `@Named("outputCapabilities")
Supplier<…>` provided by the CLI invoker. Cleaner separation, but a bigger
refactor.
**The `@Experimental` annotation**
Since this is genuinely new API surface and detection is inherently
best-effort (especially the `ExecTerminalProvider` inconclusive-negative case),
`@Experimental` on the interface is important. Worth noting explicitly.
**Impact on the logging PR chain**
As noted in the earlier review, this PR needs a rebase on
`feature/console-modes` (#13180) regardless. Once there,
`determineConsoleMode()` should feed directly into the capabilities supplier —
`CONSOLE` / `REDIRECTED` / `FILE` can be decided from the console mode + `-l`
flag without touching JLine at all in most cases; the JLine probe is only the
fallback when no explicit `--console` flag was given.
Summary: the direction is the right one — `maven-api-core` interface +
`maven-impl` default + CLI override. The main decision point is whether to keep
the `install()`/cleanup pattern (mutable singleton) or replace it with a
`Supplier` injected by the CLI layer. Happy to look at either approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
