You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/04/30 09:20:07 UTC
[GitHub] [maven-common-artifact-filters] cstamas opened a new pull request #18: [MSHARED-988] Drop uses of Plexus Logger
cstamas opened a new pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18
https://issues.apache.org/jira/browse/MSHARED-988
Replace it with Slf4j, that logger we use in maven.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] asfgit closed pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] cstamas commented on pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#issuecomment-830025291
bah, undoing ea560c629bcf8f2ed2ae52276a6bc5be2ed30d53 as this PR is to "de-plexusize", but that commit is about something else, it belongs to https://github.com/apache/maven-common-artifact-filters/pull/17
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] cstamas commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624654698
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
Um, I don't quite understand:
* we are making sure THIS module is plexus-free, to achieve that we are making sure no plexus enters it's classpath (hence the exclusions)
* "transitive dependencies might be needed", due above, this implies that some consumer of this module "might still need" plexus? My stance then is that consumer dependencies are badly set up (using something that is not a dependency), wouldn't in that case https://maven.apache.org/plugins/maven-dependency-plugin/analyze-mojo.html trigger an alarm? So when consumer dependencies are changed (version of m-common-a-f upped), at the same time that consumer should be a) de-plexusized as well, or b) have it's deps properly set up.
My 5 cents.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] elharo commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624676196
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
We are not trying to make sure there is no plexus in the classpath. We are trying to make sure this plugin does not introduce a dependency on plexus, but this plugin should not attempt to control the dependencies of its own dependencies. Doing so like this risks breaking them in confusing and hard-to-debug ways. If one of this plugin's dependencies introduces plexus, then that needs to be addressed in that dependency, not here.
Exclusions are risky and confusing. They should be reserved only for very well defined and understood conflicts, most often because two different artifacts are otherwise supplying the same classes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] michael-o commented on pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#issuecomment-830834992
I somewhat agree with @elharo If we have Plexus as a direct dependecy, nuke it, but we cannot control trans deps from here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] cstamas commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624499901
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
Because this pr is to "de-plexus". Upcoming change will move these to provided scope, as at runtime these ARE provided.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] elharo edited a comment on pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
elharo edited a comment on pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#issuecomment-830606106
running through jenkins: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-common-artifact-filters/job/MSHARED-988/
will merge if it passes
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] cstamas commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624680594
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
ack, then assume exclusions were there to make sure (during dev) that this project does not depend on any Plexus class (directly or indirectly, as in class hierarchy). As this project depends on maven-core and many other maven internals, I added exclusion (as even Maven 4 still brings plexus). So, will remove exclusions as PR builds ok with it in place, hence it proves the point that this project is plexus-free.
Still, my stance is that downstream projects are simply wrong if they **expect** plexus as transitive dependency from this project, which, given this dependency is meant to be consumed as maven plugin or maven extension dependency is already granted even with Maven 4.
IMO, exclusions are "necessary evil", as plexus originates (and will originate) from maven-core, and it will be the LAST project to have plexus removed. But until then, we must make sure all "downstream" projects like this one, are plexus-free, and is prepared to the moment where dependency on maven-core will NOT automatically mean "plexus is here!"
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] elharo commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624565053
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
It should only deplexus direct dependencies. Transitive dependencies might be needed and shouldn't be touched.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] elharo commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624488513
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
why the exclusion? I'd expect this change to resolve a conflict, not introduce one that needs to be excluded. Absent a conflict, we should pull in transitive dependencies since we can expect that this dependency depends on that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] cstamas commented on pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#issuecomment-830024438
@elharo now the project is p-c-d free
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] cstamas commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624680594
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
ack, then assume exclusions were there to make sure (during dev) that this project does not depend on any Plexus class (directly or indirectly, as in class hierarchy). As this project depends on maven-core and many other maven internals, I added exclusion (as even Maven 4 still brings plexus). So, will remove exclusions as PR builds ok with it in place, hence it proves the point that this project is plexus-free.
Still, my stance is that downstream projects are simply wrong if they **expect** plexus as transitive dependency from this project, which, given this dependency is meant to be consumed as maven plugin or maven extension dependency is already granted even with Maven 4.
IMO, exclusions are "necessary evil", as plexus originates (and will originate) from maven-core, and it will be the LAST project to have plexus removed. But until then, we must make sure all "downstream" projects like this one, are plexus-free, and is prepared for the moment when dependency on maven-core will NOT automatically mean "plexus is here!"
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] elharo commented on pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#issuecomment-830606106
running thorugh jenkins: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-common-artifact-filters/job/MSHARED-988/
will merge if it passes
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-common-artifact-filters] cstamas commented on a change in pull request #18: [MSHARED-988] Drop uses of Plexus Logger
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #18:
URL: https://github.com/apache/maven-common-artifact-filters/pull/18#discussion_r624654698
##########
File path: pom.xml
##########
@@ -108,11 +114,23 @@
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
+ <exclusions>
Review comment:
Um, I don't quite understand:
* we are making sure THIS module is plexus-free, to achieve that we are making sure no plexus enters it's classpath (hence the exclusions)
* "transitive dependencies might be needed", due above, this implies that some consumer of this module "might still need" plexus? My stance then is that consumer dependencies are badly set up (it uses something that is not a direct dependency), wouldn't in that case https://maven.apache.org/plugins/maven-dependency-plugin/analyze-mojo.html trigger an alarm? So when consumer dependencies are changed (version of m-common-a-f upped), at the same time that consumer should be a) de-plexusized as well, or b) have it's deps properly set up.
My 5 cents.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org