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:
> 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?
The level choosen in the `mvnd` example is _warn_ because that's it's
semantic. 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 _debug_ statement 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 message. 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]
