You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/08/09 06:38:59 UTC
[PR] Migrate internal plumbing DEBUG statements to TRACE level [maven]
gnodet opened a new pull request, #12714:
URL: https://github.com/apache/maven/pull/12714
## Summary
- Migrates 70+ DEBUG log statements to TRACE in 6 areas that produce excessive noise under `-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)
- Adds `TRACE` to `Slf4jConfiguration.Level` enum so the logging system supports `-Dmaven.logger.defaultLogLevel=trace` as a system property for Maven core developers
- `DEBUG` (`-X`) remains the right level for plugin development; `TRACE` is for Maven core developers diagnosing framework internals
## Motivation
The TRACE level was added in the logging-foundation PR to separate two audiences:
- **DEBUG** — plugin developers and power users diagnosing plugin behavior
- **TRACE** — Maven core developers tracing internal framework mechanics
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 `-X` today and drowns out the signal plugin developers actually want.
## What stays at DEBUG
- Plugin parameter resolution, mojo configuration/loading (useful for plugin devs)
- `mvnup` upgrade diagnostics
- CLI bootstrap messages (uses `o.a.m.api.cli.Logger` which doesn't have TRACE)
## Files changed (18 files, +108/-105)
| Area | Files | Changes |
|---|---|---|
| Lifecycle engine | `LifecycleDebugLogger`, `BuildPlanExecutor`, `MultiThreadedBuilder` | 32 statements |
| Cache internals | `DefaultRequestCache`, `CacheConfigurationResolver` | 22 statements |
| Classrealm | `DefaultClassRealmManager` | 8 statements |
| Resolver | `DefaultArtifactDescriptorReader`, relocation sources, `TypeDeriver` | 8 statements |
| Model builder | `DefaultModelBuilder` | 8 statements |
| Plugin resolution | `DefaultPluginVersionResolver`, `DefaultPluginPrefixResolver` | 6 statements |
| Infrastructure | `Slf4jConfiguration`, `MavenSimpleConfiguration`, `CliUtils` | TRACE level support |
| Tests | `DefaultClassRealmManagerTest`, `LookupInvokerLoggingTest` | Updated mocks |
## Test plan
- [x] All modified modules compile cleanly
- [x] `DefaultClassRealmManagerTest` updated to verify `trace()` calls instead of `debug()`
- [x] `LookupInvokerLoggingTest` updated for new TRACE enum value
- [x] All tests pass in isolation (batch-mode flakiness in `MavenInvokerTest` is pre-existing)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate internal plumbing DEBUG statements to TRACE level [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #12714:
URL: https://github.com/apache/maven/pull/12714#discussion_r4046302810
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1766,13 +1580,9 @@ Model doReadFileModel(Set<Path> activeModelReads) throws ModelBuilderException {
Path rootDirectory;
boolean rootDirectoryFromSession = false;
setSource(modelSource.getLocation());
- logger.debug("Reading file model from " + modelSource.getLocation());
+ logger.trace("Reading file model from " + modelSource.getLocation());
Path sourcePath = modelSource.getPath();
- // Use toAbsolutePath().normalize() for consistent path identity in activeModelReads.
- // This must match the normalization used in getEnhancedProperties() guard check
- // to prevent StackOverflowError from path representation mismatches (GH-12598).
- Path normalizedPath =
- sourcePath != null ? sourcePath.toAbsolutePath().normalize() : null;
+ Path normalizedPath = sourcePath != null ? sourcePath.normalize() : null;
Review Comment:
⚠️ **Regression (GH-12598): `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 in `activeModelReads`. This must match the normalization used in `getEnhancedProperties()` 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.
```suggestion
Path normalizedPath = sourcePath != null ? sourcePath.toAbsolutePath().normalize() : null;
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -892,11 +747,8 @@ private Map<String, String> getEnhancedProperties(Model model, Path rootDirector
// Also skip if the root model is already being read in an outer call frame
// to prevent StackOverflowError when a project has an internal parent in a
// subdirectory with CI-friendly ${revision} and a .mvn/ root marker (GH-12301).
- // Use toAbsolutePath().normalize() for the guard check to handle paths
- // obtained via different representations (e.g., symlinks, relative segments).
if (isParentWithinRootDirectory(rootModelPath, rootDirectory)
- && !activeModelReads.contains(
- rootModelPath.toAbsolutePath().normalize())) {
+ && !activeModelReads.contains(rootModelPath.normalize())) {
Review Comment:
⚠️ **Regression (GH-12301/GH-12598): `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.
```suggestion
&& !activeModelReads.contains(rootModelPath.toAbsolutePath().normalize())) {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -291,91 +289,24 @@ List<RemoteRepository> getExternalRepositories() {
// Contains both GAV coordinates (groupId:artifactId:version) and file paths
final Set<String> parentChain;
- // Sticky across derive(): true for a session that is itself resolving a dependency
- // (ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY), or that was derived, directly
- // or transitively, from such a session -- for instance a dependency's own parent POM.
- // Kept separate from request.getRequestType() because a parent lookup always derives a
- // CONSUMER_PARENT request regardless of what kind of session triggered it, which would
- // otherwise lose the distinction this flag preserves.
- final boolean externalOrigin;
-
ModelBuilderSessionState(ModelBuilderRequest request) {
this(
request.getSession(),
request,
new DefaultModelBuilderResult(request, ProblemCollector.create(request.getSession())),
new Graph(),
new ConcurrentHashMap<>(64),
- ConcurrentHashMap.newKeySet(),
- new ConcurrentHashMap<>(),
List.of(),
repos(request),
repos(request),
- new LinkedHashSet<>(),
- isExternalOrigin(request));
+ new LinkedHashSet<>());
}
static List<RemoteRepository> repos(ModelBuilderRequest request) {
- List<RemoteRepository> repos = request.getRepositories() != null
- ? request.getRepositories()
- : request.getSession().getRemoteRepositories();
- return mergeRepositoriesById(repos);
- }
-
- /**
- * Merges repositories that share the same ID by combining their policies.
- * This handles the case where mirror injection produces multiple repository
- * entries with the same mirror ID but different snapshot/release policies
- * (e.g., when both "central" and a profile-defined repo are mirrored to the
- * same mirror, producing two entries with the mirror's ID but different policies).
- * Without this merge, policy deduplication in the resolver can drop the snapshot
- * policy, causing SNAPSHOT parent resolution to fail (MNG-12769).
- */
- private static List<RemoteRepository> mergeRepositoriesById(List<RemoteRepository> repos) {
- if (repos.size() <= 1) {
- return List.copyOf(repos);
- }
- LinkedHashMap<String, RemoteRepository> byId = new LinkedHashMap<>();
- boolean hasDuplicates = false;
- for (RemoteRepository repo : repos) {
- RemoteRepository existing = byId.putIfAbsent(repo.getId(), repo);
- if (existing != null) {
- hasDuplicates = true;
- byId.put(repo.getId(), mergeRepositoryPolicies(existing, repo));
- }
- }
- return hasDuplicates ? List.copyOf(byId.values()) : List.copyOf(repos);
- }
-
- /**
- * Merges two repositories with the same ID by combining their policies.
- * For each policy type (release/snapshot), the result is enabled if either
- * input has it enabled. URL, proxy, authentication, and other properties
- * are preserved from the dominant (first) repository.
- */
- private static RemoteRepository mergeRepositoryPolicies(RemoteRepository dominant, RemoteRepository recessive) {
- if (dominant instanceof DefaultRemoteRepository d && recessive instanceof DefaultRemoteRepository r) {
- org.eclipse.aether.repository.RemoteRepository dr = d.getRepository();
- org.eclipse.aether.repository.RemoteRepository rr = r.getRepository();
-
- boolean mergeSnapshots =
- rr.getPolicy(true).isEnabled() && !dr.getPolicy(true).isEnabled();
- boolean mergeReleases =
- rr.getPolicy(false).isEnabled() && !dr.getPolicy(false).isEnabled();
-
- if (mergeSnapshots || mergeReleases) {
- org.eclipse.aether.repository.RemoteRepository.Builder builder =
- new org.eclipse.aether.repository.RemoteRepository.Builder(dr);
- if (mergeSnapshots) {
- builder.setSnapshotPolicy(rr.getPolicy(true));
- }
- if (mergeReleases) {
- builder.setReleasePolicy(rr.getPolicy(false));
- }
- return new DefaultRemoteRepository(builder.build());
- }
- }
- return dominant;
+ return List.copyOf(
+ request.getRepositories() != null
+ ? request.getRepositories()
Review Comment:
⚠️ **Regression (MNG-12769): `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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate internal plumbing DEBUG statements to TRACE level [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #12714:
URL: https://github.com/apache/maven/pull/12714#discussion_r4293130553
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/logging/Slf4jConfiguration.java:
##########
@@ -29,6 +29,7 @@ public interface Slf4jConfiguration {
* Level
*/
enum Level {
+ TRACE,
Review Comment:
Adding `TRACE` here requires updating all `Slf4jConfiguration` implementations. `MavenSimpleConfiguration` is updated ✅ but `LogbackConfiguration` and `Log4j2Configuration` both use `default -> ERROR` — `Level.TRACE` will silently map to ERROR on those backends.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org