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

Alter session for version discovery #965

Closed

Conversation

cstamas
Copy link

@cstamas cstamas commented May 26, 2023

Alter session used for discovery, pinpoint ONLY to processed artifact to update ALWAYS. This would fix all the problems reported here #959 and here https://issues.apache.org/jira/browse/MRESOLVER-363 just like -U does (and people do it by reflex). But this is more correct way, as -U is global and refreshes too much.

Maven behaviour (w/o any user settings, so OOTB) is to apply
"never" policies to metadata, hence versions plugin will make
Maven download ONCE the metadata, and NEVER again. Unless
users use `-U` that in turn is TOO MUCH, as whole maven session
is being updated in that case (it is global option).

Instead, make versions pluging derive session from global, and
set update policy ALWAYS but pinpointing that ONLY to the
given artifact being discovered.
@cstamas
Copy link
Author

cstamas commented May 26, 2023

@slawekjaranowski @slachiewicz @ajarmoniuk ping

@andrzejj0
Copy link
Contributor

andrzejj0 commented May 26, 2023

I wish I were a maintainer, but for the time being I can't approve the workflow 😉

EDIT I can actually.

@@ -273,11 +274,15 @@ public ArtifactVersions lookupArtifactVersions(
repositories = emptyList();
}

// in future sessions may become AutoCloseable, so all this would go into try-with-resource block
Copy link
Member

Choose a reason for hiding this comment

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

This should be marked with TODO then

Copy link
Author

Choose a reason for hiding this comment

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

unsure, i just left it here. IMHO is not TODO as session is NOT YET AutoCloseable, may be 1.11.x or maybe 2.0 but nobody can tell WHEN it will be. OTOH, "TODO" would mean "do it if you can", but it cannot be done today.

Copy link
Member

Choose a reason for hiding this comment

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

NOTE?

@cstamas
Copy link
Author

cstamas commented May 26, 2023

Am not a maintainer either here, so I will just leave this here for @slawekjaranowski or anyone else to handle. Comment may be removed, I just added it like "head up", but it may happen in near or far future (I cannot tell).

@slawekjaranowski
Copy link
Member

I'm not sure if it is good way, after it every execute plugin will cause download every metadata again.

For projects with many dependencies it can be an issue.

@cstamas
Copy link
Author

cstamas commented May 26, 2023

@slawekjaranowski every metadata? only targeted, only artifact that is in call. Maybe vrsions plugin could control, and have "own" -U instead? To reiterate: only plugins (for display-plugin-updates) or dependencies (for display-plugin-dependencies) are affected, nothing else. IMHO, it is up to user, will he invoke this on some top of the reactor, of locally for single module. One thing for sure, this is much-much better than invoking plugin with mvn -U

Point with mvn -U is that it is global, so will download "every metadata maven touches" (so literally EVERY in build), while this change downloads ONLY the metadata for artifact

@cstamas
Copy link
Author

cstamas commented May 26, 2023

@slawekjaranowski or if I misunderstood you, and you mean "will refresh metadata for maven-clean-plugin in EVERY module of the possibly 100 module build? so perform 100 refreshes?" That is not the case, as resolver refreshes once per session, so yes, it will refresh once in first module clean plugin is found, and from then on (in remaining 99 modules) cached results are used.

See https://github.com/apache/maven-resolver/blob/maven-resolver-1.9.10/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManager.java#L267

@slawekjaranowski
Copy link
Member

As I see issue from #959 is that we don't have or is wrong property for last modified in properties file.
According to https://github.com/apache/maven-resolver/pull/292/files#diff-51ca9aed78d3713d9818b753db3b9e9629743a7e773292e686f93950fca4c2b0R103 in such case default policy for repository is used.

Because default central plugin repository has policy never - https://github.com/apache/maven/blob/6f136ef4d2b4938e939189e9d2e3e0faa11a20e9/maven-model-builder/src/main/resources/org/apache/maven/model/pom-4.0.0.xml#L53
we try to override policy for whole session for all repositories.

Last I did improvement for repository with never policy to use daily #957 - maybe it will be good enough is such case.

@slawekjaranowski
Copy link
Member

I try to remove properties from resolver-status.properties

version 2.15.0 - doesn't refresh metadata
current master version - refresh once and update resolver-status.properties

@cstamas
Copy link
Author

cstamas commented May 26, 2023

Cool then, was unaware that this plugin overrides configured policy.
I am still convinced, that is PR (conditionally, like some -Dforce flag) would be useful for plugin (as aside of daily, user has no other means to say "update now!" than mvn -U which is too much).

But am fine, so if it works as you tested, am fine with it.

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.

4 participants