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 2022/01/29 16:07:39 UTC
[GitHub] [maven] laeubi opened a new pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
laeubi opened a new pull request #667:
URL: https://github.com/apache/maven/pull/667
This allows maven-(core-) extensions to supply `WorkspaceReader` as it is possible with `AbstractMavenLifecycleParticipant` currently it is only possible with some workarounds:
1. One needs to register a `AbstractMavenLifecycleParticipant` in a maven-core-extension
2. In the `afterSessionStart` cast the `RepositorySystemSession` to `DefaultRepositorySystemSession`
3. Call `setWorkspaceReader` and take care not to replace an already exiting one
With this change it is possible to simply register WorkspaceReader component directly and Maven takes care of all the rest.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MNG-XXX] SUMMARY`, where you replace `MNG-XXX`
and `SUMMARY` with the appropriate JIRA issue. Best practice is to use the JIRA issue
title in the pull request title and in the first line of the commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #667:
URL: https://github.com/apache/maven/pull/667#issuecomment-1037351577
@laeubi Please address the open comments before I am merge this.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] asfgit closed pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #667:
URL: https://github.com/apache/maven/pull/667
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r805196635
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
Review comment:
Done
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -463,51 +484,68 @@ private void validateLocalRepository( MavenExecutionRequest request )
}
}
- private Collection<AbstractMavenLifecycleParticipant> getLifecycleParticipants( Collection<MavenProject> projects )
+ private <T> Collection<T> getExtensionComponents( Collection<MavenProject> projects, Class<T> role )
{
- Collection<AbstractMavenLifecycleParticipant> lifecycleListeners = new LinkedHashSet<>();
+ Collection<T> foundComponents = new LinkedHashSet<>();
ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader();
try
{
try
{
- lifecycleListeners.addAll( container.lookupList( AbstractMavenLifecycleParticipant.class ) );
+ foundComponents.addAll( container.lookupList( role ) );
}
catch ( ComponentLookupException e )
{
// this is just silly, lookupList should return an empty list!
- logger.warn( "Failed to lookup lifecycle participants: " + e.getMessage() );
+ logger.warn( "Failed to lookup " + role + ": " + e.getMessage() );
}
- Collection<ClassLoader> scannedRealms = new HashSet<>();
+ foundComponents.addAll( getProjectScopedExtensionComponents( projects, role ) );
+ }
+ finally
+ {
+ Thread.currentThread().setContextClassLoader( originalClassLoader );
Review comment:
Done
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
+ List<WorkspaceReader> workspaceReaders = new ArrayList<WorkspaceReader>();
+ // 1) Reactor workspace reader
+ workspaceReaders.add( container.lookup( WorkspaceReader.class, ReactorReader.HINT ) );
+ // 2) Repository system session scoped workspace reader
+ WorkspaceReader repoWorkspaceReader = repoSession.getWorkspaceReader();
+ if ( repoWorkspaceReader != null )
+ {
+ workspaceReaders.add( repoWorkspaceReader );
+ }
+ // 3) .. n) project scoped workspace reader
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ if (workspaceReaders.contains( workspaceReader )) {
Review comment:
Done
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r795803848
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,37 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
- private void afterSessionStart( MavenSession session )
+ private void finishRepositorySystemSession( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ WorkspaceReader reactorWorkspace = container.lookup( WorkspaceReader.class, ReactorReader.HINT );
+ //
+ // Desired order of precedence for local artifact repositories
+ //
+ // Reactor
+ // Workspace
+ // User Local Repository
+ //
+
+ // we only lookup the project scoped ones here, lookup at system level will also find the system components
+ // (like ReactorReader) but that does not work well as these are special handled!
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ repoSession.setWorkspaceReader( ChainedWorkspaceReader.newInstance( repoSession.getWorkspaceReader(),
Review comment:
done
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,37 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
- private void afterSessionStart( MavenSession session )
+ private void finishRepositorySystemSession( MavenSession session, DefaultRepositorySystemSession repoSession )
Review comment:
done
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r800203026
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
+ List<WorkspaceReader> workspaceReaders = new ArrayList<WorkspaceReader>();
+ // 1) Reactor workspace reader
+ workspaceReaders.add( container.lookup( WorkspaceReader.class, ReactorReader.HINT ) );
+ // 2) Repository system session scoped workspace reader
+ WorkspaceReader repoWorkspaceReader = repoSession.getWorkspaceReader();
+ if ( repoWorkspaceReader != null )
+ {
+ workspaceReaders.add( repoWorkspaceReader );
+ }
+ // 3) .. n) project scoped workspace reader
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ if (workspaceReaders.contains( workspaceReader )) {
Review comment:
Spaces around parens and curly on newline
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r795408823
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,37 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
- private void afterSessionStart( MavenSession session )
+ private void finishRepositorySystemSession( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ WorkspaceReader reactorWorkspace = container.lookup( WorkspaceReader.class, ReactorReader.HINT );
+ //
+ // Desired order of precedence for local artifact repositories
+ //
+ // Reactor
+ // Workspace
+ // User Local Repository
+ //
+
+ // we only lookup the project scoped ones here, lookup at system level will also find the system components
+ // (like ReactorReader) but that does not work well as these are special handled!
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ repoSession.setWorkspaceReader( ChainedWorkspaceReader.newInstance( repoSession.getWorkspaceReader(),
Review comment:
Sure that is possible, I just adopted the "style" currently used, but ChainedWorkspaceReader accepts also an array, I can adjust ChainedWorkspaceReader to combine if any of the provided ones is already a chained one. Let me know if you think this should be done.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #667:
URL: https://github.com/apache/maven/pull/667#issuecomment-1024949029
I have uploaded my "HelloWorld" here: https://github.com/eclipse/tycho/pull/586/files
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #667:
URL: https://github.com/apache/maven/pull/667#issuecomment-1037351577
@laeubi Please address the open comments before I am merge this.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] asfgit closed pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #667:
URL: https://github.com/apache/maven/pull/667
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r800189566
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -463,51 +484,68 @@ private void validateLocalRepository( MavenExecutionRequest request )
}
}
- private Collection<AbstractMavenLifecycleParticipant> getLifecycleParticipants( Collection<MavenProject> projects )
+ private <T> Collection<T> getExtensionComponents( Collection<MavenProject> projects, Class<T> role )
{
- Collection<AbstractMavenLifecycleParticipant> lifecycleListeners = new LinkedHashSet<>();
+ Collection<T> foundComponents = new LinkedHashSet<>();
ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader();
try
{
try
{
- lifecycleListeners.addAll( container.lookupList( AbstractMavenLifecycleParticipant.class ) );
+ foundComponents.addAll( container.lookupList( role ) );
}
catch ( ComponentLookupException e )
{
// this is just silly, lookupList should return an empty list!
- logger.warn( "Failed to lookup lifecycle participants: " + e.getMessage() );
+ logger.warn( "Failed to lookup " + role + ": " + e.getMessage() );
}
- Collection<ClassLoader> scannedRealms = new HashSet<>();
+ foundComponents.addAll( getProjectScopedExtensionComponents( projects, role ) );
+ }
+ finally
+ {
+ Thread.currentThread().setContextClassLoader( originalClassLoader );
Review comment:
Why is this necessary now, the TCCL is never overridden here?
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
+ List<WorkspaceReader> workspaceReaders = new ArrayList<WorkspaceReader>();
+ // 1) Reactor workspace reader
+ workspaceReaders.add( container.lookup( WorkspaceReader.class, ReactorReader.HINT ) );
+ // 2) Repository system session scoped workspace reader
+ WorkspaceReader repoWorkspaceReader = repoSession.getWorkspaceReader();
+ if ( repoWorkspaceReader != null )
+ {
+ workspaceReaders.add( repoWorkspaceReader );
+ }
+ // 3) .. n) project scoped workspace reader
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ if (workspaceReaders.contains( workspaceReader )) {
Review comment:
Formatting
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
Review comment:
before quiering
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r800201523
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -463,51 +484,68 @@ private void validateLocalRepository( MavenExecutionRequest request )
}
}
- private Collection<AbstractMavenLifecycleParticipant> getLifecycleParticipants( Collection<MavenProject> projects )
+ private <T> Collection<T> getExtensionComponents( Collection<MavenProject> projects, Class<T> role )
{
- Collection<AbstractMavenLifecycleParticipant> lifecycleListeners = new LinkedHashSet<>();
+ Collection<T> foundComponents = new LinkedHashSet<>();
ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader();
try
{
try
{
- lifecycleListeners.addAll( container.lookupList( AbstractMavenLifecycleParticipant.class ) );
+ foundComponents.addAll( container.lookupList( role ) );
}
catch ( ComponentLookupException e )
{
// this is just silly, lookupList should return an empty list!
- logger.warn( "Failed to lookup lifecycle participants: " + e.getMessage() );
+ logger.warn( "Failed to lookup " + role + ": " + e.getMessage() );
}
- Collection<ClassLoader> scannedRealms = new HashSet<>();
+ foundComponents.addAll( getProjectScopedExtensionComponents( projects, role ) );
+ }
+ finally
+ {
+ Thread.currentThread().setContextClassLoader( originalClassLoader );
Review comment:
I think this is obsolete because its done in the method call already, I'll adjust this.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #667:
URL: https://github.com/apache/maven/pull/667#issuecomment-1024941870
> I'm not done yet with the review (I just need a bigger screen which I don't have available at this moment); I'd like to take a more thorough look soon.
Thanks for looking into this, there is currently an issue that this also finds the `org.apache.maven.ReactorReader` I'm currently try to find out how to avoid this as this obviously is not what one wants :-/
I'll try to cleanup the unrelated parts, it just seems the Eclipse-Formater likes to format more than desired 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r795409079
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -463,51 +482,59 @@ private void validateLocalRepository( MavenExecutionRequest request )
}
}
- private Collection<AbstractMavenLifecycleParticipant> getLifecycleParticipants( Collection<MavenProject> projects )
+ private <T> Collection<T> getExtensionComponents( Collection<MavenProject> projects, Class<T> role )
{
- Collection<AbstractMavenLifecycleParticipant> lifecycleListeners = new LinkedHashSet<>();
+ Collection<T> foundComponents = new LinkedHashSet<>();
ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader();
try
{
try
{
- lifecycleListeners.addAll( container.lookupList( AbstractMavenLifecycleParticipant.class ) );
+ foundComponents.addAll( container.lookupList( role ) );
}
catch ( ComponentLookupException e )
{
// this is just silly, lookupList should return an empty list!
- logger.warn( "Failed to lookup lifecycle participants: " + e.getMessage() );
+ logger.warn( "Failed to lookup " + role + ": " + e.getMessage() );
}
- Collection<ClassLoader> scannedRealms = new HashSet<>();
+ foundComponents.addAll( getProjectScopedExtensionComponents( projects, role ) );
+ }
+ finally
+ {
+ Thread.currentThread().setContextClassLoader( originalClassLoader );
+ }
+
+ return foundComponents;
+ }
+
+ protected <T> Collection<T> getProjectScopedExtensionComponents( Collection<MavenProject> projects, Class<T> role )
+ {
+
+ Collection<T> foundComponents = new LinkedHashSet<>();
+ Collection<ClassLoader> scannedRealms = new HashSet<>();
+
+ for ( MavenProject project : projects )
+ {
+ ClassLoader projectRealm = project.getClassRealm();
- for ( MavenProject project : projects )
+ if ( projectRealm != null && scannedRealms.add( projectRealm ) )
{
- ClassLoader projectRealm = project.getClassRealm();
+ Thread.currentThread().setContextClassLoader( projectRealm );
Review comment:
Good point, I'll check 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r795070819
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -19,6 +19,25 @@
* under the License.
*/
+import javax.inject.Inject;
Review comment:
Could you please revert this change as it seems unrelated to what you want to achieve.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r795410247
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,37 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
- private void afterSessionStart( MavenSession session )
+ private void finishRepositorySystemSession( MavenSession session, DefaultRepositorySystemSession repoSession )
Review comment:
I just found that in maven 4 (in contrast to 3.8.x) everything is extracted into a method, if you look a bit down the line, there is the `RepositorySystemSession#setReadOnly()` so I thought this might contain every action before the session is made readonly/finalized, but of course I would choose another name if it is appropriate, so let me know!
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r795526006
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,37 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
- private void afterSessionStart( MavenSession session )
+ private void finishRepositorySystemSession( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ WorkspaceReader reactorWorkspace = container.lookup( WorkspaceReader.class, ReactorReader.HINT );
+ //
+ // Desired order of precedence for local artifact repositories
+ //
+ // Reactor
+ // Workspace
+ // User Local Repository
+ //
+
+ // we only lookup the project scoped ones here, lookup at system level will also find the system components
+ // (like ReactorReader) but that does not work well as these are special handled!
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ repoSession.setWorkspaceReader( ChainedWorkspaceReader.newInstance( repoSession.getWorkspaceReader(),
Review comment:
Okay, ChainedWorkspaceReader is not part of this repository should I suggest a change in ChainedWorkspaceReader or should I handle it in MavenDefault?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r800201417
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
+ List<WorkspaceReader> workspaceReaders = new ArrayList<WorkspaceReader>();
+ // 1) Reactor workspace reader
+ workspaceReaders.add( container.lookup( WorkspaceReader.class, ReactorReader.HINT ) );
+ // 2) Repository system session scoped workspace reader
+ WorkspaceReader repoWorkspaceReader = repoSession.getWorkspaceReader();
+ if ( repoWorkspaceReader != null )
+ {
+ workspaceReaders.add( repoWorkspaceReader );
+ }
+ // 3) .. n) project scoped workspace reader
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ if (workspaceReaders.contains( workspaceReader )) {
Review comment:
I applied the Maven Formater and check style did not complain can you explain whats wrong with formatting 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r805196635
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
Review comment:
Done
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -463,51 +484,68 @@ private void validateLocalRepository( MavenExecutionRequest request )
}
}
- private Collection<AbstractMavenLifecycleParticipant> getLifecycleParticipants( Collection<MavenProject> projects )
+ private <T> Collection<T> getExtensionComponents( Collection<MavenProject> projects, Class<T> role )
{
- Collection<AbstractMavenLifecycleParticipant> lifecycleListeners = new LinkedHashSet<>();
+ Collection<T> foundComponents = new LinkedHashSet<>();
ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader();
try
{
try
{
- lifecycleListeners.addAll( container.lookupList( AbstractMavenLifecycleParticipant.class ) );
+ foundComponents.addAll( container.lookupList( role ) );
}
catch ( ComponentLookupException e )
{
// this is just silly, lookupList should return an empty list!
- logger.warn( "Failed to lookup lifecycle participants: " + e.getMessage() );
+ logger.warn( "Failed to lookup " + role + ": " + e.getMessage() );
}
- Collection<ClassLoader> scannedRealms = new HashSet<>();
+ foundComponents.addAll( getProjectScopedExtensionComponents( projects, role ) );
+ }
+ finally
+ {
+ Thread.currentThread().setContextClassLoader( originalClassLoader );
Review comment:
Done
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,39 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
+ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ // Desired order of precedence for workspace reader before query the local artifact repositories
+ List<WorkspaceReader> workspaceReaders = new ArrayList<WorkspaceReader>();
+ // 1) Reactor workspace reader
+ workspaceReaders.add( container.lookup( WorkspaceReader.class, ReactorReader.HINT ) );
+ // 2) Repository system session scoped workspace reader
+ WorkspaceReader repoWorkspaceReader = repoSession.getWorkspaceReader();
+ if ( repoWorkspaceReader != null )
+ {
+ workspaceReaders.add( repoWorkspaceReader );
+ }
+ // 3) .. n) project scoped workspace reader
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ if (workspaceReaders.contains( workspaceReader )) {
Review comment:
Done
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #667:
URL: https://github.com/apache/maven/pull/667#issuecomment-1024943668
> In the meantime, would you be so kind to revert the unrelated change in imports? It allows us to focus on the actual changes you're proposing. Thanks!
Done! `mvn clean verify` also runs without test or checkstyle errors now!
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #667: MNG-7400 - Allow more WorkspaceReader's to participate
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #667:
URL: https://github.com/apache/maven/pull/667#discussion_r795403559
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,37 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
- private void afterSessionStart( MavenSession session )
+ private void finishRepositorySystemSession( MavenSession session, DefaultRepositorySystemSession repoSession )
+ throws ComponentLookupException
+ {
+ WorkspaceReader reactorWorkspace = container.lookup( WorkspaceReader.class, ReactorReader.HINT );
+ //
+ // Desired order of precedence for local artifact repositories
+ //
+ // Reactor
+ // Workspace
+ // User Local Repository
+ //
+
+ // we only lookup the project scoped ones here, lookup at system level will also find the system components
+ // (like ReactorReader) but that does not work well as these are special handled!
+ for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
+ WorkspaceReader.class ) )
+ {
+ repoSession.setWorkspaceReader( ChainedWorkspaceReader.newInstance( repoSession.getWorkspaceReader(),
Review comment:
If there are multiple `WorkspaceReader` implementations, this would create a chain that is larger than necessary; each new extension would create a new `ChainedWorkspaceReader` that points to the previous `ChainedWorkspaceReader` and adds the one it just detected in the extension.
Wouldn't it make more sense to first find all the extension components and then build only one `ChainedWorkspaceReader` with all `WorkspaceReaders` found in all the extensions?
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -463,51 +482,59 @@ private void validateLocalRepository( MavenExecutionRequest request )
}
}
- private Collection<AbstractMavenLifecycleParticipant> getLifecycleParticipants( Collection<MavenProject> projects )
+ private <T> Collection<T> getExtensionComponents( Collection<MavenProject> projects, Class<T> role )
{
- Collection<AbstractMavenLifecycleParticipant> lifecycleListeners = new LinkedHashSet<>();
+ Collection<T> foundComponents = new LinkedHashSet<>();
ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader();
try
{
try
{
- lifecycleListeners.addAll( container.lookupList( AbstractMavenLifecycleParticipant.class ) );
+ foundComponents.addAll( container.lookupList( role ) );
}
catch ( ComponentLookupException e )
{
// this is just silly, lookupList should return an empty list!
- logger.warn( "Failed to lookup lifecycle participants: " + e.getMessage() );
+ logger.warn( "Failed to lookup " + role + ": " + e.getMessage() );
}
- Collection<ClassLoader> scannedRealms = new HashSet<>();
+ foundComponents.addAll( getProjectScopedExtensionComponents( projects, role ) );
+ }
+ finally
+ {
+ Thread.currentThread().setContextClassLoader( originalClassLoader );
+ }
+
+ return foundComponents;
+ }
+
+ protected <T> Collection<T> getProjectScopedExtensionComponents( Collection<MavenProject> projects, Class<T> role )
+ {
+
+ Collection<T> foundComponents = new LinkedHashSet<>();
+ Collection<ClassLoader> scannedRealms = new HashSet<>();
+
+ for ( MavenProject project : projects )
+ {
+ ClassLoader projectRealm = project.getClassRealm();
- for ( MavenProject project : projects )
+ if ( projectRealm != null && scannedRealms.add( projectRealm ) )
{
- ClassLoader projectRealm = project.getClassRealm();
+ Thread.currentThread().setContextClassLoader( projectRealm );
Review comment:
I'm far from familiar with how class loading works in Maven, but if we _change_ something here, shouldn't we _restore_ it by the end of the method (just like we do in `getExtensionComponents()`)?
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -367,11 +354,37 @@ private MavenExecutionResult doExecute( MavenExecutionRequest request, MavenSess
return result;
}
- private void afterSessionStart( MavenSession session )
+ private void finishRepositorySystemSession( MavenSession session, DefaultRepositorySystemSession repoSession )
Review comment:
The name of this method sounds a bit odd to me. Seems more like it "adds workspace readers" or something like that, than that it "finishes" something. But maybe in the context of this change, it makes sense?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org