Skip to content

[MNG-7278] Clean up core logging#562

Merged
cstamas merged 11 commits intomasterfrom
cleanup-core-logging
Oct 2, 2021
Merged

[MNG-7278] Clean up core logging#562
cstamas merged 11 commits intomasterfrom
cleanup-core-logging

Conversation

@cstamas
Copy link
Copy Markdown
Member

@cstamas cstamas commented Oct 1, 2021

We had all kind of Loggers in Core, some Plexus
injected, some acquired using SLF4J LoggerFactory,
some static, some final, etc.

This PR aligns all those uses to SLF4J finals.

Notices:

  • maven-core does NOT use Plexus Logger anymore
  • did not touch maven-compat

https://issues.apache.org/jira/browse/MNG-7278

We had all kind of Loggers in Core, some Plexus
injected, some acquired using SLF4J LoggerFactory,
some static, some final, etc.

This PR aligns all those uses to SLF4J finals.

Notices:
* maven-core does NOT use Plexus Logger anymore
* did not touch maven-compat
@cstamas cstamas self-assigned this Oct 1, 2021
Comment thread maven-plugin-api/pom.xml Outdated
@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Oct 1, 2021

Fails due ancient (1.4!!!) enforcer plugin used in ITs MavenITmng5840RelativePathReactorMatching and MavenITmng5840ParentVersionRanges

@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Oct 1, 2021

This IT change fixes ITs apache/maven-integration-testing#115

@cstamas cstamas marked this pull request as ready for review October 1, 2021 14:51
@cstamas cstamas changed the title Clean up core logging [MNG-7278] Clean up core logging Oct 1, 2021
Comment thread maven-core/src/main/java/org/apache/maven/toolchain/DefaultToolchainManager.java Outdated
This is maven4, so breaking change (moved deprecated class)
should be okay.

Once this merged, we should really un-deprecate mojo Log class.

public BuilderCommon()
{
this.logger = LoggerFactory.getLogger( getClass() );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ctor is needed for container, look below the 2nd ctor

*/
DefaultToolchainManager( Logger logger )
{
this.logger = requireNonNull( logger );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why UTs need this....

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not touch UTs, I merely "cleaned up core logging". Still, added a TODO, as in default toolchain manager (and private) components there are some peculiarities: annotated component should not be extended (is bad practice), so IMO this needs some refactoring...

*/
DefaultToolchainManagerPrivate( Logger logger )
{
super( logger );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not touch UTs, I merely "cleaned up core logging". Still, added a TODO, as in default toolchain manager (and private) components there are some peculiarities: annotated component should not be extended (is bad practice), so IMO this needs some refactoring...

@Deprecated //TODO used by the Enforcer plugin (cstamas: not anymore, since 3.0.0)
public PluginParameterExpressionEvaluator( MavenSession session, MojoExecution mojoExecution,
PathTranslator pathTranslator, Logger logger, MavenProject project,
PathTranslator pathTranslator, MavenProject project,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change for maven-enforcer-plugin pre 3.0.0... IMO, we should just drop this (long time deprecated) constructor, as 3.x plugins are requirement for a long time, and this is Maven4.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will handle this in https://issues.apache.org/jira/browse/MNG-7279 to not mish-mash this PR

private final Logger logger;
private final PlexusContainer container;
private final Logger logger = LoggerFactory.getLogger( getClass() );
private final PlexusContainer container; // TODO not used? Then remove
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from master, container seems unused...

implements ToolchainManager
{
final Logger logger;
protected final Logger logger; // TODO this class is extended, needs refactoring
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two classes DefaultToolchainManager and DefaultToolchainManagerPrivate needs refactor, both has TODOs for it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cstamas cstamas merged commit c3962c1 into master Oct 2, 2021
@cstamas cstamas deleted the cleanup-core-logging branch October 2, 2021 18:33
@jira-importer
Copy link
Copy Markdown

Resolve #8159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants