Conversation
84d77ae to
b6a409b
Compare
c0b643f to
2a79603
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean mechanical migration of 70+ debug statements to trace level. MavenSimpleConfiguration, CliUtils, and tests are all correctly updated to handle the new Level.TRACE enum value.
Missing TRACE handling in two logging backends:
The new TRACE enum value is correctly handled in MavenSimpleConfiguration (case TRACE -> "trace"), CliUtils (case TRACE, DEBUG ->), and the test, but two other setRootLoggerLevel implementations were not updated:
Log4j2Configuration.java(line 32-36):default -> "error"silently mapsTRACEto"error"— should addcase TRACE -> "trace"LogbackConfiguration.java(line 34-37):default -> ch.qos.logback.classic.Level.ERRORsilently mapsTRACEto ERROR — should addcase TRACE -> ch.qos.logback.classic.Level.TRACE
Both are live code — registered in META-INF/maven/slf4j-configuration.properties as runtime-selected implementations. While Level.TRACE is not yet wired to a CLI option (limiting immediate impact), the inconsistency should be fixed to prevent a silent bug when trace-level CLI support lands.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean mechanical migration of 70+ internal DEBUG statements to TRACE level. All switch expressions, guard conditions, and tests are correctly updated.
Non-blocking observations:
LifecycleDebugLogger.debug(String)now callslogger.trace(), making the method name misleading. However, the class is non-public API and renaming would expand the scope unnecessarily.- Several trace log calls use string concatenation instead of parameterized logging (pre-existing, not introduced by this PR).
- Pre-existing arithmetic bug in
DefaultModelBuilder.java:2722:afterSize - beforeSizeproduces a negative count for removed entries. Should bebeforeSize - afterSize. This PR only changed the log level.
LGTM — the migration is well-scoped: only internal plumbing is demoted to TRACE, while plugin-facing DEBUG output correctly remains at DEBUG.
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean mechanical migration of 70+ internal DEBUG statements to TRACE level across 18 files. All switch expressions, guard conditions, and tests are correctly updated.
The migration scoping is well-reasoned: only internal framework plumbing (lifecycle engine, classrealm, cache internals, resolver, model builder, plugin resolution) is demoted to TRACE. Plugin-facing DEBUG output (parameter resolution, mojo configuration) correctly remains at DEBUG, preserving the signal that plugin developers need from -X.
Note: the two prior AI-generated reviews flagged Log4j2Configuration and LogbackConfiguration as missing TRACE handling — this is a false positive since both files were removed in an earlier PR in the chain and do not exist on the target branch feature/12643-structured-problems.
Looks good overall. ✅
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
2a79603 to
6eff81f
Compare
b6a409b to
4b6969b
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean mechanical migration of 70+ internal DEBUG statements to TRACE across 18 files. The scoping is well-reasoned — only internal framework plumbing (lifecycle engine, classrealm, cache internals, resolver, model builder, plugin resolution) is demoted to TRACE, while plugin-facing DEBUG output (parameter resolution, mojo configuration) correctly remains at DEBUG.
The Slf4jConfiguration.Level enum update is correctly propagated to MavenSimpleConfiguration, and the CliUtils mapping functions correctly map TRACE to the same level as DEBUG for legacy APIs that lack a TRACE concept.
A few minor pre-existing observations (not introduced by this PR):
DefaultModelBuilder.javaline ~2722:afterSize - beforeSizeproduces a negative value when reporting "removed {} entries" — should bebeforeSize - afterSizeLifecycleDebugLogger: Several trace log calls use string concatenation instead of parameterized logging ("Project: " + ...vs"Project: {}", ...)- The
compat/maven-resolver-providerTypeDeriverstill hasdebug()calls for the same messages migrated totrace()in theimpl/version — acceptable since compat modules are legacy wrappers
LGTM — the rebase introduced no content changes and the migration is correct throughout.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
4b6969b to
3b439d0
Compare
6eff81f to
6c40a9e
Compare
3b439d0 to
5f53884
Compare
6c40a9e to
7a6b6ad
Compare
5f53884 to
a964976
Compare
7a6b6ad to
2d77564
Compare
a964976 to
33f16e8
Compare
2d77564 to
0d2db3b
Compare
33f16e8 to
0661b09
Compare
0d2db3b to
b282ea5
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The logging migration itself (DEBUG → TRACE for 70+ statements) is clean and well-scoped. The Slf4jConfiguration.Level.TRACE enum addition, switch propagation in CliUtils/MavenSimpleConfiguration, and test updates are all correct.
However, the DefaultModelBuilder.java diff includes several non-logging functional changes that revert or weaken fixes that were already present on the base branch (feature/12643-structured-problems). These need to be addressed before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| // to prevent StackOverflowError from path representation mismatches (GH-12598). | ||
| Path normalizedPath = | ||
| sourcePath != null ? sourcePath.toAbsolutePath().normalize() : null; | ||
| Path normalizedPath = sourcePath != null ? sourcePath.normalize() : null; |
There was a problem hiding this comment.
sourcePath.normalize() without toAbsolutePath()
The base branch used sourcePath.toAbsolutePath().normalize() here to ensure a canonical, absolute path is stored in activeModelReads. The comment that was removed explicitly explained why:
Use
toAbsolutePath().normalize()for consistent path identity inactiveModelReads. This must match the normalization used ingetEnhancedProperties()guard check to prevent StackOverflowError from path representation mismatches (GH-12598).
With only .normalize(), a relative sourcePath (e.g. when modelSource.getPath() returns a relative Path) will be stored as-is. The guard in getEnhancedProperties at line 751 uses rootModelPath.normalize() — also without toAbsolutePath(). If the two paths represent the same file but one is absolute and one is relative, the contains() check returns false and the StackOverflow reappears on projects that triggered GH-12598.
| Path normalizedPath = sourcePath != null ? sourcePath.normalize() : null; | |
| Path normalizedPath = sourcePath != null ? sourcePath.toAbsolutePath().normalize() : null; |
| if (isParentWithinRootDirectory(rootModelPath, rootDirectory) | ||
| && !activeModelReads.contains( | ||
| rootModelPath.toAbsolutePath().normalize())) { | ||
| && !activeModelReads.contains(rootModelPath.normalize())) { |
There was a problem hiding this comment.
rootModelPath.normalize() without toAbsolutePath()
The base branch guard was rootModelPath.toAbsolutePath().normalize() to match the identity of entries put into activeModelReads by doReadFileModel. That comment was deleted here:
Use
toAbsolutePath().normalize()for the guard check to handle paths obtained via different representations (e.g., symlinks, relative segments).
The rootModelPath comes from modelProcessor.locateExistingPom(rootDirectory). If that returns a relative or symlinked path while the entry in activeModelReads is an absolute path (or vice versa), the cycle guard fires when it shouldn't, or misses when it should fire.
| && !activeModelReads.contains(rootModelPath.normalize())) { | |
| && !activeModelReads.contains(rootModelPath.toAbsolutePath().normalize())) { |
| return dominant; | ||
| return List.copyOf( | ||
| request.getRepositories() != null | ||
| ? request.getRepositories() |
There was a problem hiding this comment.
mergeRepositoriesById removed
The base branch's repos() method called mergeRepositoriesById() which merged duplicate repo entries (same ID, different snapshot/release policies) that mirror injection can produce. The Javadoc explained:
Without this merge, policy deduplication in the resolver can drop the snapshot policy, causing SNAPSHOT parent resolution to fail (MNG-12769).
This PR replaces repos() with a plain List.copyOf(...), silently reintroducing the MNG-12769 bug for projects that use mirrors with split release/snapshot policies.
The fix should be re-applied here, or moved to a layer below ModelBuilderSessionState if the intent is to clean up its scope.
2 participants
Summary
-X: lifecycle engine (reactor plan dumps, step scheduling), classrealm (realm creation/population/imports), cache internals (config resolution, access stats), resolver (descriptor filtering, relocation), model builder (cache clearing, profile activation), and plugin resolution (version/prefix tracing)TRACEtoSlf4jConfiguration.Levelenum so the logging system supports-Dmaven.logger.defaultLogLevel=traceas a system property for Maven core developersDEBUG(-X) remains the right level for plugin development;TRACEis for Maven core developers diagnosing framework internalsMotivation
The TRACE level was added in the logging-foundation PR to separate two audiences:
Without this migration, TRACE exists but nothing emits at it, making the distinction theoretical. This PR populates it with the noisiest internal plumbing output that dominates
-Xtoday and drowns out the signal plugin developers actually want.What stays at DEBUG
mvnupupgrade diagnosticso.a.m.api.cli.Loggerwhich doesn't have TRACE)Files changed (18 files, +108/-105)
LifecycleDebugLogger,BuildPlanExecutor,MultiThreadedBuilderDefaultRequestCache,CacheConfigurationResolverDefaultClassRealmManagerDefaultArtifactDescriptorReader, relocation sources,TypeDeriverDefaultModelBuilderDefaultPluginVersionResolver,DefaultPluginPrefixResolverSlf4jConfiguration,MavenSimpleConfiguration,CliUtilsDefaultClassRealmManagerTest,LookupInvokerLoggingTestPR chain
mvnlogviewerTest plan
DefaultClassRealmManagerTestupdated to verifytrace()calls instead ofdebug()LookupInvokerLoggingTestupdated for new TRACE enum valueMavenInvokerTestis pre-existing)🤖 Generated with Claude Code