Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNG-7432] Resolver session contains non-MavenWorkspaceReader #695

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

laeubi
Copy link

@laeubi laeubi commented Mar 15, 2022

Signed-off-by: Christoph Läubrich christoph@laeubi-soft.de

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue 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 successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement 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
you have to acknowledge this by using the following check-box.

@laeubi
Copy link
Author

laeubi commented Mar 15, 2022

FYI @famod @olamy @michael-o

@laeubi
Copy link
Author

laeubi commented Mar 15, 2022

I have copied the code from org.eclipse.aether.util.repository.ChainedWorkspaceReader because it is a final class.

I'm using a delegation to the original org.eclipse.aether.util.repository.ChainedWorkspaceReadernow.

@laeubi laeubi force-pushed the quickfix_quarkus branch 2 times, most recently from 6aea07d to 3a72f60 Compare March 15, 2022 09:55
@famod
Copy link
Contributor

famod commented Mar 15, 2022

I've just created a ticket for this: https://issues.apache.org/jira/browse/MNG-7432

@laeubi laeubi changed the title Quick Fix for https://github.com/quarkusio/quarkus/pull/24285 [MNG-7432] Dependencies from profile not picked up via -P<profileName> Mar 18, 2022
Signed-off-by: Christoph Läubrich <christoph@laeubi-soft.de>
@laeubi
Copy link
Author

laeubi commented Mar 18, 2022

@michael-o do you like to review this?

@michael-o
Copy link
Member

@michael-o do you like to review this?

I need to understand the cause first because there seems to be some quirk, no?

@laeubi
Copy link
Author

laeubi commented Mar 18, 2022

Not sure, at least this does restores the previous behavior.
The problem is caused by the following (we have noticed something similar in Tycho as well):

  1. MavenWorkspaceReader has the method findModel

  2. But the chained reader has not implemented this

  3. The reactor reader implement it on the other hand

  4. at two places in maven it is explicitly queried for the MavenWorkspaceReader#findModel what produces slightly different results:

  5. Lets assume we have a dependency build by the current reactor and referenced in another project, then the model might be read again with different outcome (e.g. because of https://www.mojohaus.org/flatten-maven-plugin/plugin-info.html used)

  6. When using profiles, it seems to modify the in memory model returned by MavenWorkspaceReader#findModel, but if that is read from disk the additions from the profile are missing

The fix do the following:

  1. return the ReactorWorspaceReader if it is the only WSR (as in Maven 3.8.4)
  2. implement MavenWorkspaceReader#findModel in a chained mode

this should fix all this issues.

@basil
Copy link

basil commented Mar 19, 2022

@olamy The regression introduced in Maven 3.8.5 in 6f14196, which is resolved in this unmerged and unreleased pull request, is causing all Jenkins core PR builds to fail.

@basil
Copy link

basil commented Mar 19, 2022

A more minimal (and therefore likely less controversial) alternative to the fix proposed in this PR is:

diff --git a/maven-core/src/main/java/org/apache/maven/DefaultMaven.java b/maven-core/src/main/java/org/apache/maven/DefaultMaven.java
index 56a42b724..965345394 100644
--- a/maven-core/src/main/java/org/apache/maven/DefaultMaven.java
+++ b/maven-core/src/main/java/org/apache/maven/DefaultMaven.java
@@ -377,8 +377,15 @@ private void setupWorkspaceReader( MavenSession session, DefaultRepositorySystem
             }
             workspaceReaders.add( workspaceReader );
         }
-        WorkspaceReader[] readers = workspaceReaders.toArray( new WorkspaceReader[0] );
-        repoSession.setWorkspaceReader( new ChainedWorkspaceReader( readers ) );
+        if ( workspaceReaders.size() == 1 )
+        {
+            repoSession.setWorkspaceReader( workspaceReaders.get( 0 ) );
+        }
+        else
+        {
+            WorkspaceReader[] readers = workspaceReaders.toArray( new WorkspaceReader[0] );
+            repoSession.setWorkspaceReader( new ChainedWorkspaceReader( readers ) );
+        }
 
     }

This fixes the problem for me by restoring the old behavior from Maven 3.8.4.

@laeubi
Copy link
Author

laeubi commented Mar 20, 2022

This fixes the problem for me

but will instantly fail once another workspace reader is registered again ;-)

@basil
Copy link

basil commented Mar 20, 2022

I do not really know or care what a workspace reader is. I just want my builds to work again.

@laeubi
Copy link
Author

laeubi commented Mar 20, 2022

I do not really know or care what a workspace reader is. I just want my builds to work again.

You can use maven 3.8.4 in the meantime.

@basil
Copy link

basil commented Mar 20, 2022

I think that writing "restoring the old behavior from Maven 3.8.4" should have made it clear that I already knew that.

@laeubi
Copy link
Author

laeubi commented Mar 20, 2022

I think that writing "restoring the old behavior from Maven 3.8.4" should have made it clear that I already knew that.

Sure but if you simply "do not really care" the easiest and fastest way is using 3.8.4. For 3.8.5 support for additional Readers was added because there are people who care, and applying a fix that would only "restoring the old behavior" (that was already broken but probably never noticed) won't help much.

I also don't see that the fix is controversial just that @michael-o want to better understand the cause, so if you can provide additional information why your build fails e.g. by providing a minimal reproducer, integration or unit test or you can confirm that it is the same problem described by @famod then it might help to make more progress here.

@basil
Copy link

basil commented Mar 20, 2022

so if you can provide

Sorry, I lack the time and interest to pursue this further. I have already reverted back to Maven 3.8.4.

@jglick
Copy link
Contributor

jglick commented Mar 21, 2022

I suppose

if ( readers.length == 1 )
{
return readers[0];
}
in this PR effectively restores previous behavior in the normal case, just like #695 (comment)?

@laeubi
Copy link
Author

laeubi commented Mar 21, 2022

I suppose

if ( readers.length == 1 )
{
return readers[0];
}

in this PR effectively restores previous behavior in the normal case, just like #695 (comment)?

No, this is just a shorthand in case there is only one WSR.

@laeubi
Copy link
Author

laeubi commented Mar 29, 2022

@michael-o just wondering inf we can proceed here? If its fine for 3.8.x I'll prepare PRs for 2.9.x and 4.x as well ... I also wonder if it would be suitable to release a 3.8.6 version or do we have to wait for 2.9.0?

Copy link
Contributor

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Seems like a straightforward fix. If I understand correctly, one or both of

if ( workspace instanceof MavenWorkspaceReader )
{
model = ( (MavenWorkspaceReader) workspace ).findModel( pomArtifact );
if ( model != null )
{
return model;
}
}
or
if ( workspace instanceof MavenWorkspaceReader )
{
model = ( (MavenWorkspaceReader) workspace ).findModel( RepositoryUtils.toArtifact( artifact ) );
}
broke in 3.8.5? So merging this towards 3.8.6 would restore the 3.8.4 behavior.

@laeubi
Copy link
Author

laeubi commented Mar 29, 2022

If I understand correctly, one or both of

Exactly!

@michael-o
Copy link
Member

Is there an IT for this? If there would be, I'd check this and add for 3.8.6+.

@laeubi
Copy link
Author

laeubi commented Mar 29, 2022

Is there an IT for this? If there would be, I'd check this and add for 3.8.6+.

The jira has a link to a reproducer maybe this could be integrated an in itest or give a hint how to create one?

@jglick
Copy link
Contributor

jglick commented Mar 29, 2022

For reference, the Jenkins build error was at

@famod
Copy link
Contributor

famod commented Mar 30, 2022

@laeubi

Is there an IT for this? If there would be, I'd check this and add for 3.8.6+.

The jira has a link to a reproducer maybe this could be integrated an in itest or give a hint how to create one?

ITs are here: https://github.com/apache/maven-integration-testing

public static WorkspaceReader of( Collection<WorkspaceReader> workspaceReaderCollection )
{
WorkspaceReader[] readers = workspaceReaderCollection.toArray( new WorkspaceReader[0] );
if ( readers.length == 1 )
Copy link
Member

@cstamas cstamas Apr 2, 2022

Choose a reason for hiding this comment

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

I'd personally just remove this length check, and make it always wrapped (is simpler).

Copy link
Author

Choose a reason for hiding this comment

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

I think the complexity is manageable given that in most cases there is only one WSP and we save useless loop calls in that case when it is used across the maven build.

Copy link
Member

@cstamas cstamas Apr 2, 2022

Choose a reason for hiding this comment

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

Still, the check could happen then on collection, not after conversion to array, no? nvm, am fine with it as it is

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just used the "toArray" as we accept a collection and then getting the first element from collection requires to iterate over the collection, but if size is larger requires an toArray as well... in the end it dosn't make much a difference given that this only happens once per maven invocation.

@cstamas
Copy link
Member

cstamas commented Apr 2, 2022

Created UT that IMHO should cover the issue #706
Please reuse it freely, no need to keep author etc (just lift it from there)

This UT makes sure that DefaultMaven sets up proper type
of WorkspaceReader in RepositorySystemSession.

For example, this UT fails with current maven-3.8.x and
maven-3.9.x branches.
@laeubi laeubi requested a review from cstamas April 4, 2022 14:30
@laeubi
Copy link
Author

laeubi commented Apr 4, 2022

@cstamas I have now fetched your test as well into this PR.

@cstamas
Copy link
Member

cstamas commented Apr 7, 2022

@michael-o so what's blocking here? Should I take over?

@michael-o
Copy link
Member

Feel free to take over the entire ticket handling with IT, etc.

@cstamas
Copy link
Member

cstamas commented Apr 7, 2022

Ok, but IMHO no need for IT here, as UT covers nicely (again: when resolver runs "within maven", the session.workspaceReader MUST be instanceof MavenWorkspaceReader to get expected -- stuff from reactor -- result, that's all)

@michael-o
Copy link
Member

Ok, but IMHO no need for IT here, as UT covers nicely (again: when resolver runs "within maven", the session.workspaceReader MUST be instanceof MavenWorkspaceReader to get expected -- stuff from reactor -- result, that's all)

So, you even wouldn't consider to absorb the reproducer as an IT?

@cstamas
Copy link
Member

cstamas commented Apr 7, 2022

As I commented on JIRA, I consider the reproducer WRONG USECASE, and if we lift that into ITs we would put it in concrete...

@michael-o
Copy link
Member

As I commented on JIRA, I consider the reproducer WRONG USECASE, and if we lift that into ITs we would put it in concrete...

I go with that explanation

@cstamas cstamas changed the title [MNG-7432] Dependencies from profile not picked up via -P<profileName> [MNG-7432] Resolver session contains non-MavenWorkspaceReader Apr 8, 2022
@cstamas cstamas merged commit 957b5e3 into apache:maven-3.8.x Apr 8, 2022
cstamas added a commit that referenced this pull request Apr 8, 2022
As Resolver session contains non-MavenWorkspaceReader, the reactor models (already resolved w/ profiles applied) are re-built when using Resolver within Mojo, instead to get them via ReactorReader as expected. The rebuilt models will lack explicit (-P on CLI) profiles applied, as resolver itself is not maven aware, hence there is no way to "tell" resolver to apply them. Building reactor models w/ profiles applied is NOT done using resolver, but by Maven when loading up reactor, as profiles are NOT applied for downstream transitive dependencies (see discussion on MNG-1388 why).

Signed-off-by: Christoph Läubrich <christoph@laeubi-soft.de>
Co-authored-by: Christoph Läubrich <christoph@laeubi-soft.de>
Co-authored-by: Tamas Cservenak <tamas@cservenak.net>
gnodet pushed a commit to gnodet/maven that referenced this pull request Apr 12, 2022
…#695)

As Resolver session contains non-MavenWorkspaceReader, the reactor models (already resolved w/ profiles applied) are re-built when using Resolver within Mojo, instead to get them via ReactorReader as expected. The rebuilt models will lack explicit (-P on CLI) profiles applied, as resolver itself is not maven aware, hence there is no way to "tell" resolver to apply them. Building reactor models w/ profiles applied is NOT done using resolver, but by Maven when loading up reactor, as profiles are NOT applied for downstream transitive dependencies (see discussion on MNG-1388 why).

Signed-off-by: Christoph Läubrich <christoph@laeubi-soft.de>
Co-authored-by: Christoph Läubrich <christoph@laeubi-soft.de>
Co-authored-by: Tamas Cservenak <tamas@cservenak.net>
@jglick jglick mentioned this pull request Jun 3, 2022
@gnodet gnodet mentioned this pull request May 24, 2023
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.

6 participants