Skip to content

[MNG-8646] Make IT tests use "outer" Mimir caching as well #2176

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

Merged
merged 16 commits into from
Mar 26, 2025

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Mar 21, 2025

As it turned out, ITs were not using Mimir as they redefined Maven Central to allow snapshots, and that made Mimir not handle the IT used Maven Central (while those did not contain snapshots, it was still pointing at canonical Maven Central URL).

Fixing this revealed several other (related or unrelated) issues:

  • MavenProject was NPE prone regarding remote repositories handling
  • MappedList was NPE prone regarding null values (emitted by MavenProject remote repositories)
    several ProjectModelBuilder (detailed below) had no remote repositories set anymore (as central is not in super POM anymore)
  • some ITs revaled ReactorReader issues as well

The org.apache.maven.project.DefaultProjectBuilder issues fixed:

  • model request type CONSUMER_PARENT is used for non-locally built projects (ie. plugins invoked with direct mojo invocation), this seems strange. This skips CI friendly replacement for example as shows in Maven ITs
  • remote repositories are injected ONLY when injecting lifecycle plugins (processPlugins=true). This was ok when root pom was defining default repository, but that's not the case anymore, and this results in projects built that has 0 remote reposes.

At the end of the day, all issues were fixed but one IT that we declared "invalid" was disabled (w/ remark about why's).


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

ITs used settings.xml redefines Central to allow
snapshots, hence Mimir refuses to touch IT used
Central (while still being loaded). This PR now
fixes settings.xml to NOT do this.

OTOH, there are two ITs that needs now fix, one
is wrongly assuming it can get snapshots from
Central (cannot anymore), and other is basically
affected by change in ordering.

Signed-off-by: Tamas Cservenak <[email protected]>
@cstamas cstamas self-assigned this Mar 21, 2025
@cstamas cstamas requested a review from gnodet March 21, 2025 12:44
@cstamas cstamas mentioned this pull request Mar 21, 2025
@gnodet
Copy link
Contributor

gnodet commented Mar 21, 2025

Two CI friendly IT fails, 4 failures in total:

  • MavenITmng5895CIFriendlyUsageWithPropertyTest (2 test method)
  • MavenITmng6090CIFriendlyTest (2 test method)

The failures are caused by the m-assembly-p trying to create MavenProject for the projects being built. Unfortunately, the request is typed as CONSUMER_PARENT by the heuristic, as if we were trying to download a dependency's parent.
This causes the CI friendly version replacement to be skipped.

Not sure what's the best way to fix that yet though...

resolveAndReadParentExternally:1035, DefaultModelBuilder$ModelBuilderSessionState (org.apache.maven.impl.model)
resolveParent:880, DefaultModelBuilder$ModelBuilderSessionState (org.apache.maven.impl.model)
readParent:848, DefaultModelBuilder$ModelBuilderSessionState (org.apache.maven.impl.model)
readEffectiveModel:1153, DefaultModelBuilder$ModelBuilderSessionState (org.apache.maven.impl.model)
buildEffectiveModel:785, DefaultModelBuilder$ModelBuilderSessionState (org.apache.maven.impl.model)
build:241, DefaultModelBuilder$ModelBuilderSessionImpl (org.apache.maven.impl.model)
build:378, DefaultProjectBuilder$BuildSession (org.apache.maven.project)
build:455, DefaultProjectBuilder$BuildSession (org.apache.maven.project)
build:187, DefaultProjectBuilder (org.apache.maven.project)
build:180, DefaultProjectBuilder (org.apache.maven.project)
addDependencySet:161, AddDependencySetsTask (org.apache.maven.plugins.assembly.archive.task)
execute:122, AddDependencySetsTask (org.apache.maven.plugins.assembly.archive.task)
execute:88, DependencySetAssemblyPhase (org.apache.maven.plugins.assembly.archive.phase)
createArchive:173, DefaultAssemblyArchiver (org.apache.maven.plugins.assembly.archive)
execute:522, AbstractAssemblyMojo (org.apache.maven.plugins.assembly.mojos)
execute:60, SingleAssemblyMojo (org.apache.maven.plugins.assembly.mojos)
executeMojo:146, DefaultBuildPluginManager (org.apache.maven.plugin)
doExecute2:339, MojoExecutor (org.apache.maven.lifecycle.internal)
doExecute:310, MojoExecutor (org.apache.maven.lifecycle.internal)
execute:214, MojoExecutor (org.apache.maven.lifecycle.internal)
execute:179, MojoExecutor (org.apache.maven.lifecycle.internal)
run:168, MojoExecutor$1 (org.apache.maven.lifecycle.internal)
execute:39, DefaultMojosExecutionStrategy (org.apache.maven.plugin)
execute:165, MojoExecutor (org.apache.maven.lifecycle.internal)
buildProject:110, LifecycleModuleBuilder (org.apache.maven.lifecycle.internal)
buildProject:76, LifecycleModuleBuilder (org.apache.maven.lifecycle.internal)
build:60, SingleThreadedBuilder (org.apache.maven.lifecycle.internal.builder.singlethreaded)
execute:123, DefaultLifecycleStarter (org.apache.maven.lifecycle.internal)
doExecute:311, DefaultMaven (org.apache.maven)
doExecute:225, DefaultMaven (org.apache.maven)
execute:149, DefaultMaven (org.apache.maven)
doExecute:462, MavenInvoker (org.apache.maven.cling.invoker.mvn)
execute:100, MavenInvoker (org.apache.maven.cling.invoker.mvn)
execute:81, MavenInvoker (org.apache.maven.cling.invoker.mvn)
doInvoke:165, LookupInvoker (org.apache.maven.cling.invoker)
invoke:135, LookupInvoker (org.apache.maven.cling.invoker)
run:76, ClingSupport (org.apache.maven.cling)
main:51, MavenCling (org.apache.maven.cling)

@cstamas
Copy link
Member Author

cstamas commented Mar 21, 2025

Heuristic:

                    ModelBuilderRequest.RequestType type = pomFile != null
                                    && this.request.isProcessPlugins()
                                    && this.request.getValidationLevel() == ModelBuildingRequest.VALIDATION_LEVEL_STRICT
                            ? ModelBuilderRequest.RequestType.BUILD_EFFECTIVE
                            : ModelBuilderRequest.RequestType.CONSUMER_PARENT;

Am unsure why "process plugins" matters here, I'd rather go: pomFile != null (so is checkout, so to say "current project") then assume build effective, and if no file present, then assume is "remote/external".

@cstamas
Copy link
Member Author

cstamas commented Mar 21, 2025

Formatting. But the change is too "big" IMO.
Maybe just "if pom being built is from reactor, do not build consumer?". Still, that means the process plugins value is ignored?

@gnodet
Copy link
Contributor

gnodet commented Mar 21, 2025

Formatting. But the change is too "big" IMO. Maybe just "if pom being built is from reactor, do not build consumer?". Still, that means the process plugins value is ignored?

So m-assembly-p does the following

        return new DefaultProjectBuildingRequest(configSource.getMavenSession().getProjectBuildingRequest())
                .setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL)
                .setProcessPlugins(false);

so we need to remove both conditions, else it will have no effect.

The actual problem is that there's no different made by m-assembly-p between loading a pom from the repository (as a consumer) and loading from the projects being built. Maybe that's the one that needs to be fixed ?

@gnodet
Copy link
Contributor

gnodet commented Mar 21, 2025

Formatting. But the change is too "big" IMO. Maybe just "if pom being built is from reactor, do not build consumer?". Still, that means the process plugins value is ignored?

So m-assembly-p does the following

        return new DefaultProjectBuildingRequest(configSource.getMavenSession().getProjectBuildingRequest())
                .setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL)
                .setProcessPlugins(false);

so we need to remove both conditions, else it will have no effect.

The actual problem is that there's no different made by m-assembly-p between loading a pom from the repository (as a consumer) and loading from the projects being built. Maybe that's the one that needs to be fixed ?

I'm going to experiment with apache/maven-assembly-plugin#217

@gnodet
Copy link
Contributor

gnodet commented Mar 21, 2025

Formatting. But the change is too "big" IMO. Maybe just "if pom being built is from reactor, do not build consumer?". Still, that means the process plugins value is ignored?

So m-assembly-p does the following

        return new DefaultProjectBuildingRequest(configSource.getMavenSession().getProjectBuildingRequest())
                .setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL)
                .setProcessPlugins(false);

so we need to remove both conditions, else it will have no effect.
The actual problem is that there's no different made by m-assembly-p between loading a pom from the repository (as a consumer) and loading from the projects being built. Maybe that's the one that needs to be fixed ?

I'm going to experiment with apache/maven-assembly-plugin#217

It seems to work locally for me. @cstamas could you double check when you have some time ?

@cstamas
Copy link
Member Author

cstamas commented Mar 24, 2025

I dislike it. It means we need to modify legacy code base to make it work with Maven 4? Are we sure we cannot make it work by improving the heuristic instead?

cstamas and others added 10 commits March 24, 2025 13:47
And add some bugfixes, npe fixes and alike.
* initProject sets remote reposes (was not before)
* parent reposes are ADDED not replacing (as now models are not sure to have remote repository as it is not in super pom anymore)
* injectPlugins method made clear it RE-sets remote reposes (but that is invoked only for build MP instances)
This reverts commit 2c2a0ff.
@cstamas cstamas marked this pull request as ready for review March 26, 2025 19:04
@cstamas cstamas changed the title [IT] Fix to make Mimir cache for ITs [MNG-8646] Fixes to make Mimir cache be used for ITs Mar 26, 2025
@cstamas cstamas changed the title [MNG-8646] Fixes to make Mimir cache be used for ITs [MNG-8646] Make IT tests use "outer" Mimir caching as well Mar 26, 2025
@cstamas cstamas merged commit d6e9524 into apache:master Mar 26, 2025
19 checks passed
@cstamas cstamas deleted the make-mimir-cache-for-ITs branch March 26, 2025 20:54
@github-actions github-actions bot added this to the 4.0.0-rc-4 milestone Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants