gnodet commented on a change in pull request #616:
URL: https://github.com/apache/maven/pull/616#discussion_r783019478
##########
File path:
maven-embedder/src/main/java/org/apache/maven/cli/internal/BootstrapCoreExtensionManager.java
##########
@@ -114,14 +123,42 @@ private CoreExtensionEntry createExtension( CoreExtension
extension, List<Artifa
{
String realmId =
"coreExtension>" + extension.getGroupId() + ":" +
extension.getArtifactId() + ":" + extension.getVersion();
- ClassRealm realm = classWorld.newRealm( realmId, null );
- log.debug( "Populating class realm " + realm.getId() );
- realm.setParentRealm( parentRealm );
+ final ClassRealm realm = classWorld.newRealm( realmId, null );
+ Set<String> providedArtifacts = Collections.emptySet();
+ String classLoadingStrategy = extension.getClassLoadingStrategy();
+ if ( STRATEGY_PARENT_FIRST.equals( classLoadingStrategy ) )
+ {
+ realm.importFrom( parentRealm, "" );
+ }
+ else if ( STRATEGY_PLUGIN.equals( classLoadingStrategy ) )
+ {
+ coreExports.getExportedPackages().forEach( ( p, cl ) ->
realm.importFrom( cl, p ) );
+ providedArtifacts = coreExports.getExportedArtifacts();
+ }
+ else if ( STRATEGY_SELF_FIRST.equals( classLoadingStrategy ) )
+ {
+ realm.setParentRealm( parentRealm );
+ }
+ else
+ {
+ throw new IllegalArgumentException( "Unsupported class-loading
strategy '"
+ + classLoadingStrategy + "'. Supported values are: " +
STRATEGY_PARENT_FIRST
+ + ", " + STRATEGY_PLUGIN + " and " + STRATEGY_SELF_FIRST );
+ }
+ log.debug( "Populating class realm {}", realm.getId() );
for ( Artifact artifact : artifacts )
{
- File file = artifact.getFile();
- log.debug( " Included " + file );
- realm.addURL( file.toURI().toURL() );
+ String id = artifact.getGroupId() + ":" + artifact.getArtifactId();
+ if ( providedArtifacts.contains( id ) )
+ {
+ log.debug( " Excluded {}", id );
+ }
+ else
+ {
+ File file = artifact.getFile();
+ log.debug( " Included {}", file );
Review comment:
> I consider this as an antipattern. Warn is warn, regardless which
level is enabled.
I agree.
> I am not really convinced by this.
> I would have expected this:
>
> ```
> if ( log.isTraceEnabled() )
> {
> log.trace( " Included {} located at {}", id, file );
> }
> else
> {
> log.debug( " Included {}", id );
> }
> ```
>
> The message remains the same, but the granularity changes.
This exactly contradicts your statement above which is "warn is warn".
Here, a _debug_ is logged either at _trace_ *or* _debug_ which is really wrong
to me.
> Why do you I have to enable debug to see more information with the same
warn level?
Same as here. The level choosen in the `mvnd` example is _warn_ because
that's what it is. The exception stack trace however, is not necessary to
understand the warning usually. But in case you need to have more detailed
information, you raise the log level to debug, and you have more information
provided. But that does not mean the _warning_ becomes a _trace_ as you said.
I think we have the exact same use case here. The fact that an artifact is
included or excluded is _debug_ level info. If you need more, in this case the
exact file location, you can raise the log level, but the semantic about the
fact that the artifact is included or not does not change, nor should its log
level.
If we can't agree on that solution, I'd go for the alternative using two
statements, one at debug and one at trace level.
```
log.debug( " Included {}", id );
log.trace( " -> {}", file );
```
--
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]
