michael-o commented on a change in pull request #616:
URL: https://github.com/apache/maven/pull/616#discussion_r784645105
##########
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.
You are right.
> > Why do you I have to enable debug to see more information with the same
warn level?
>
> The level chosen in the `mvnd` example is _warn_ because that's its
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.
Understand.
> 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 );
> ```
That's not really better because it is split now in two lines which makes it
even harder to grep.
Counter questions:
* Why not display the path always with debug?
* Why display the path with excludes, but not with inlcudes?
--
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]
