refactor: Put MIMA in action #1604

cstamas · 2023-03-31T10:28:20Z

Re-use MIMA in ArtifactResolver, and properly handle repoSys shutdown.

About MIMA https://github.com/maveniverse/mima

maxandersen · 2023-03-31T11:00:52Z

related to https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/maven.203.2E9.20.28and.20past.29.20compatability/near/345682355

cstamas · 2023-03-31T13:57:39Z

Just now spotted: while the build and tests pass OK (!) locally, the resulting distro is totally broken, and for some reason is missing almost all of the (runtime) classes and many more.... 😞

Yup: https://imperceptiblethoughts.com/shadow/configuration/minimizing/ is pulling out all the bits

quintesse · 2023-03-31T17:23:19Z

Personally I'd be happy to use something made by people who actually understand Maven, because what we did with JBang was just cobble different things from different sources and different versions together, without understanding things fully, and just pray it works.

cstamas · 2023-03-31T17:31:52Z

As I stand currently, in this moment, will have to probably create step-by-step PRs, as I keep discovering "issues" (bugs maybe?), and have to solve them to keep going. For example very first bug I saw is re localRepository: Settings.getLocalMavenRepo() never returns null, it's confidence is infinite 😄 and returns "factory default" as last resort. Then later, ArtifactResolver tries to use settings.xml but will not, as override (for wrong) local repo is in place... for example on my WS I never use "default" local repo location: makes it easy to spot buggy software...

cstamas · 2023-03-31T17:37:15Z

Also, am currently playing on "two fronts", since MIMA 1.0.1 release added some new stuff (like maveniverse/mima@5719b7b) so my goal is to make MIMA provide "fully configured" miniMaven, while JBang only have to care about it's own specific overrides. (like JBANG_REPO is)...

quintesse · 2023-03-31T18:08:59Z

Settings.getLocalMavenRepo() never returns null, it's confidence is infinite

Well exactly because it returns a "factory default" it never returns null, that should be correct, right?

cstamas · 2023-03-31T18:16:10Z

Well exactly because it returns a "factory default" it never returns null, that should be correct, right?

No, that is not correct. User may have settings.xml in default place (~/.m2/settings.xml) that says that Maven local repo is actually at /foo/bar/repo.
Moreover, since https://issues.apache.org/jira/browse/MNG-7590 (present in 3.8.7+, 3.9.0+, ...) the resolver configuration may be injected from settings active profile properties, and root of local repo is not only at /foo/bar/repo but may be split as well (see https://maven.apache.org/resolver/local-repository.html#split-local-repository).

cstamas · 2023-03-31T21:00:19Z

Just FTR I pushed this change to different branch as am getting a bit lost, or am just digging too deep:
https://github.com/cstamas/jbang/compare/mima...cstamas:jbang:mima-step-1?expand=1

Goals:

manage rootContext single instance (create when needed, close it when done, TBD yet)
the context is "customized" per invocation, but am unsure can params change (ie. logging or offline), they seems mutable but per JBang invocation they seem unchanged?
this makes resolver reuse session. caches, and whole component graph, so it should be faster to resolve, especially bigger trees

quintesse · 2023-03-31T23:55:41Z

No, that is not correct. User may have settings.xml in default place (~/.m2/settings.xml) that says that Maven local repo is actually at /foo/bar/repo.

Sure, but that would still be a result, right? I mean the settings.xml can't "unset" the local repo, or can it? I do understand that the implementation is wrong for the reasons you mention, I'm just wondering if it not returning null is an issue or not.

quintesse · 2023-03-31T23:59:01Z

the context is "customized" per invocation, but am unsure can params change (ie. logging or offline), they seems mutable but per JBang invocation they seem unchanged?

Indeed, right now JBang doesn't change those parameters during an invocation, but I wrote that code to be flexible in that aspect "just in case".

quintesse · 2023-04-03T19:36:58Z

src/main/java/dev/jbang/source/Project.java

@@ -26,6 +28,10 @@
 public class Project {
 	@Nonnull
 	private final ResourceRef resourceRef;
+
+	@Nonnull


We'll have to come up with a different solution for this because Project is a model object and cannot contain anything that is not directly related top project management.

I haven't looked at it in-depth yet, but I'm guessing this would need to be reversed, perhaps have some kind of factory that given a Project can return a RootContext.

cstamas · 2023-04-28T15:19:23Z

FYI, will pick up this once resolver 1.9.10 and maven 3.9.2 are out.

quintesse · 2023-04-28T16:55:32Z

FYI, will pick up this once resolver 1.9.10 and maven 3.9.2 are out.

This was on my TODO-SOON list (like one of these days), are you saying it would be better to wait for those new versions?

cstamas · 2023-04-28T17:18:39Z

Yes, i'd wait, we are not in hurry 😄
Latest MIMA released (1.1.0) uses Resolver 1.9.8 that suffers from eventing bug (IMHO it would not affect use case in jbang), while resolver release 1.9.9 vote was canceled today by me, as another bug was found. All this is fixed, and will release latest MIMA, so MIMA, upcoming Maven 3.9.2 and upcoming Maven 4-alpha-7 will be aligned re resolver versions.

quintesse · 2023-05-04T11:29:51Z

@cstamas a question: what is the object that we should try to cache/not create too often and that we have to make sure to close?
I would assume it was the Context, but in the latest commit I see that an additional context is created using customize() and that one isn't being closed. Is that because it depends on the "parent" and it's enough to only close that one?

I'm just trying to figure out where exactly to fit the creation of the Context into our code. We have two situations where we do lookups and they use slightly different configurations and have different lifecycles.

Knowing how best to create/cache will help me select the best way forward

cstamas · 2023-05-04T11:38:22Z

Yes, ideally a Context should be used in try-with-resource, nested if you "derive"/customize context. The topmost "root" context should be kept open as long as app/jvm runs.Personally I'd make ArtifactResolver closeable, and close of it would clode the (derived/customized) context as well.

Explanation: constructing "root" (very first) context could be considered "heavy" operation, while customizing (child) context and closing it is "light" operation -- as root still keeps constructed resolver components. Customization allows to use alt local repo, enforce update from remote, change involved remote reposes, etc.

cstamas · 2023-05-04T11:46:31Z

Tomorrow vote for resolver 1.9.10 ends, will do a mima release and pick this up.

If you want to toy with it, look at latest "static" addition (I think MIMA version I use here did not yet have it), it is equivalent of deprecated ServiceLocator runtime (no guice/sisu involved, much lighter) https://github.com/maveniverse/mima/tree/main#runtimes

quintesse · 2023-05-04T13:00:46Z

Ok, because I'm basically looking for a solution that is very similar to what you had before the "A bit bigger commit". Where the resolving is almost like a kind of service that can simply be called,

Trying to pass it all the way down from the ProjectBuilder just doesn't really fit well. Even if we could get it to work it will conflict with some refactorings we have in mind.

Now, they ideal solution might be to make it part of the existing DependencyResolver mechanism, but that would mean some refactoring. Especially because none of that was written with the idea of having to clean up after itself.

So another idea (hack) I had was that we could have some singleton manager that would keep track of contexts and close them once we exit the app.

cstamas · 2023-05-04T13:16:09Z

It will work if you make it as before "A bit bigger commit". Actually, I'd revert that commit fully, as I went to completely wrong direction. So for start, doing in as it was before it would be ok (and using static uber runtime) and see the startup penalty.... and later we could improve by refactoring?

quintesse · 2023-05-04T15:46:36Z

Ok, perfect. It's what I had done locally already as a starting point. I'll push that for now and then we update to the new version when it's out and then we can compare any startup differences.

cstamas · 2023-05-04T17:48:43Z

Sisu is bad candidate for "minification", so i pushed:

change from sisu to static runtime
updated version of mima (as version used in this PR had no static runtime yet)

cstamas · 2023-05-04T17:49:36Z

Locally 7 tests fail, but mostly due "unexpected" (junit tmp) location of local repo/resolved file, while otherwise assertions looks they should pass...

quintesse · 2023-05-04T18:23:49Z

Ok, I'll take a look at those tests. Thanks!

cstamas · 2023-05-04T18:38:18Z

build.gradle

@@ -189,7 +182,11 @@ shadowJar {
 		//exclude(dependency('org.slf4j:slf4j-api:.*'))
 		exclude(dependency('org.slf4j:slf4j-nop:.*'))
 		exclude(dependency('org.jboss.logging:jboss-logging:.*'))
+		exclude(dependency('eu.maveniverse.maven.mima:.*'))
+		exclude(dependency('eu.maveniverse.maven.mima.runtime:.*'))
+		exclude(dependency('org.apache.maven.resolver:.*'))


These lines now with static-uber runtime could be removed, as minimize removed almost all of resolver (due implicit uses via JSR330 that minimize was unaware of).

Hm, given that mima.runtime goes in via ServiceLocator, maybe not.... (unsure does minimize plugin "knows" about ServiceLocator pattern, as it does not know about Sisu index either).

cstamas · 2023-05-04T18:39:08Z

build.gradle

 	}
+	append 'META-INF/sisu/javax.inject.Named'


Also unneded: already merged in static-uber runtime (unless we bring in some other Sisu stuff) -- is safe to be left as is

quintesse · 2023-05-05T09:05:10Z

Locally 7 tests fail, but mostly due "unexpected" (junit tmp) location of local repo/resolved file, while otherwise assertions looks they should pass...

@cstamas it seems this is because MIMA isn't honoring the maven.repo.local system property

quintesse · 2023-05-05T09:09:49Z

We could simply add that to Settings.getJBangLocalMavenRepoOverride() of course, I'm just mentioning this in case this is something that should be handled by MIMA or not...

cstamas · 2023-05-05T09:35:22Z

yup, not honoring maven.repo.local (or honoring it ONLY as a user property) was a bug, doing 1.1.2 release in few (as it turned out there were more system prop and env related issues): maveniverse/mima@46651eb
1.1.2 sync pending to central (20ish minutes), and then maven.repo.local should be picked up as well

cstamas · 2023-05-05T09:45:28Z

1.1.2 synced to central

w/ 1.1.2 my local test results:

BUILD SUCCESSFUL in 2m 54s
26 actionable tasks: 14 executed, 12 up-to-date
[cstamas@urnebes jbang (mima *)]$

pushing changes...

maxandersen · 2023-11-18T05:34:56Z

whoops - things broke?

DependencyResolverTest > testResolveDependenciesAltRepo(File) FAILED
    dev.jbang.cli.ExitException at DependencyResolverTest.java:126
        Caused by: org.eclipse.aether.resolution.DependencyResolutionException at DependencyResolverTest.java:126
            Caused by: org.eclipse.aether.collection.DependencyCollectionException at DependencyResolverTest.java:126
                Caused by: org.eclipse.aether.resolution.VersionRangeResolutionException at DependencyResolverTest.java:126

DependencyResolverTest > testResolveDependenciesWithAether() FAILED
    dev.jbang.cli.ExitException at DependencyResolverTest.java:115
        Caused by: org.eclipse.aether.resolution.DependencyResolutionException at DependencyResolverTest.java:115
            Caused by: org.eclipse.aether.collection.DependencyCollectionException at DependencyResolverTest.java:115
                Caused by: org.eclipse.aether.resolution.VersionRangeResolutionException at DependencyResolverTest.java:115

DependencyResolverTest > testResolveJavaFXWithAether() FAILED
    dev.jbang.cli.ExitException at DependencyResolverTest.java:108
        Caused by: org.eclipse.aether.resolution.DependencyResolutionException at DependencyResolverTest.java:108
            Caused by: org.eclipse.aether.resolution.ArtifactResolutionException at DependencyResolverTest.java:108
                Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException at DependencyResolverTest.java:108

pretty sure it passed earlier ?

maxandersen · 2023-11-18T06:00:49Z

the dependencies it fails on in case @cstamas can spot if somehow special:

org.openjfx:javafx-base:jar:mac-aarch64:18.0.2
log4j:log4j:jar:[1.2,)

cstamas · 2023-11-18T12:58:51Z

Yup, can confirm. Seems remoteRepositories are empty (so nowhere to resolve from), will look into it a bit later.

The semantics of .repositories() changed, as it was not possible to configure MIMA without remote repositories but sometimes you need that. Before, it always prepended Central. Also, there is new "op" what to do with provided list, when repositories discovered from settings.xml are considered as well.

cstamas · 2023-11-18T16:53:23Z

Pushed fix, locally passes OK: in 2.4 I changed the .repositories() semantics on builder (the ContextOverride), as before it was impossible to configure MIMA without remote repositories (and sometimes you do want that). Also, added the new "op" for handling explicitly provided list of remote repositories when considering remote repositories discovered from settings.xml as well (you usually want to prepend, to "override", that is default).

Depending on intent here (I just guessed), in case partialRepos is null, maybe more correct would be to NOT invoke ContextOverrides.Builder.repositories() method at all... (and rely on defaults)

cstamas · 2023-11-18T20:00:41Z

Btw, resolver 2.0.0-alpha-2 just released, it introduces new HTTP/2 transport based on JDK 11 java.net.http.HttpClient (that is naturally dependency-less) and boosts around 20% in download perf when target (like Central) supports HTTP/2 (it does). Speed is pretty much same as "apache" transport when target is HTTP/1.1 (like some Maven Repo Manager....).

MIMA will soon start supporting resolver 2.0.0 (probably in parallel with 1.9..x line) and once JBang upgrades to MIMA, following resolver updates will be trivial (just update MIMA).

For JBang this may be interesting, as it drops requirement for ASF httpClient, jcl-over-slf4j...

maxandersen · 2023-11-19T01:24:46Z

Can that be done in a way so when run on java 8 it will still fallback?

maxandersen · 2023-11-19T08:15:14Z

btw. about:

"Also, there is new "op" what to do with provided list,
when repositories discovered from settings.xml are considered
as well.
"

did we ever consider repos from settings.xml ? that wouldn't be what I expect...unless its purely for the case where no repos specified in jbang?

cstamas · 2023-11-19T11:07:09Z

Can that be done in a way so when run on java 8 it will still fallback?

Yes, mvn4 does exactly that:

it it runs on java8, transport defaults to "apache" (as with mvn3)
if it runs on java11+ transport defaults to "jdk" and is HTTP/2
there is also new "jetty" transport, other alternative that knows HTTP/2 (and soon even HTTP/3)

The "jdk" transport is actually multi-release JAR that is "no op" on Java8:
https://github.com/apache/maven-resolver/tree/master/maven-resolver-transport-jdk-parent

cstamas · 2023-11-19T11:10:00Z

did we ever consider repos from settings.xml ? that wouldn't be what I expect...unless its purely for the case where no repos specified in jbang?

MIMA always did consider reposes from settings.xml, and I assumed this is what you want... IF that is not the case, OP.REPLACE needs to be set and then jbang "drives" (and reposes from settings.xml are neglected/replaced with those specifified in repositories).

MIMA mimics what Maven does in a way: reposes specified in contextOverrides are like POM specified RemoteReposes, and are merged (op.PREPEND, the default) to those discovered in settings.xml. But, for flexibility, I added the "op" that can op.APPEND and op.REPLACE as well... if JBang does not want to care for settings reposes, it should use op REPLACE then...

maxandersen · 2023-11-19T11:15:35Z

It's mainly for repos - default in jbang have been nothing spécified- we add central. If you specify something you control all the repos.

I wonder if we should have a "magic" settings.xml repo name that lets user place it. Or a flag that says honor settings.xml...

cstamas · 2023-11-19T11:46:54Z

I'd consider wrong to not observe settings.xml defined reposes, but I can understand it can be overwhelming and why would someone want to ignore those. So, yes, exposing this on jbang (honor or do not honor reposes in settings.xml) with sensible default (i guess as current intent: ignore) would make sense.

maxandersen · 2023-11-19T16:38:19Z

Rationale to not honor it is reproducibility/consistency to ensure that a jbang script will run the same by default.

Totally get that it would be nice to honor settings repositories - but that I feel should be optin not opt out.

maxandersen · 2023-12-06T08:09:16Z

did we ever consider repos from settings.xml ? that wouldn't be what I expect...unless its purely for the case where no repos specified in jbang?

MIMA always did consider reposes from settings.xml, and I assumed this is what you want... IF that is not the case, OP.REPLACE needs to be set and then jbang "drives" (and reposes from settings.xml are neglected/replaced with those specifified in repositories).

MIMA mimics what Maven does in a way: reposes specified in contextOverrides are like POM specified RemoteReposes, and are merged (op.PREPEND, the default) to those discovered in settings.xml. But, for flexibility, I added the "op" that can op.APPEND and op.REPLACE as well... if JBang does not want to care for settings reposes, it should use op REPLACE then...

...i'm being blind - where is it you say we should set this in this PR to not have repos from settings.xml included?

It picks up latest Resolver 1.9.18 that causes less surprises to Windows users.

cstamas · 2023-12-06T09:21:52Z

Here you are:
ff51b8a

(sadly spotless behaves very strange).
Make sure partialReposes ALWAYS have repo (at least central if user does not set any), and set Op to REPLACE (to ignore settings.xml discovered reposes).

maxandersen · 2023-12-07T23:12:41Z

itests/test_suite.sh

@@ -52,7 +52,7 @@ rm -rf ~/.jbang/cache
 echo Testing with `which jbang`

 ## init ##
-assert "jbang init $SCRATCH/test.java"
+assert "jbang --verbose init $SCRATCH/test.java"


why this change?

IMHO can be undone, I think that was done early in this PR, probably had some issue so needed debug logs for resolver?

maxandersen · 2023-12-07T23:13:35Z

src/test/java/dev/jbang/cli/TestRun.java

@@ -521,7 +521,7 @@ void testAliasWithRepo(@TempDir File output) throws IOException {
 				"  \"aliases\": {\n" +
 				"    \"aliaswithrepo\": {\n" +
 				"      \"script-ref\": \"dummygroup:dummyart:0.1\",\n" +
-				"      \"repositories\": [ \"http://dummyrepo\" ]\n" +
+				"      \"repositories\": [ \"https://dummyrepo\" ]\n" +


is http now allowed anymore or ?

what is allowed (only HTTPS or HTTP) or not is matter of setup/config. For example if you have something like this in your settings.xml (user or system): https://github.com/apache/maven/blob/master/apache-maven/src/assembly/maven/conf/settings.xml#L160-L166 then HTTP is blocked.

maxandersen

just minor questions - all big important stuff looks resolved now!

maxandersen · 2023-12-07T23:29:23Z

Lets do this - thank you!

@all-contributors please add @cstamas for code

allcontributors · 2023-12-07T23:29:32Z

@maxandersen

I've put up a pull request to add @cstamas! 🎉

quintesse · 2023-12-07T23:37:48Z

Yay! 🎉

cstamas changed the title ~~Put MIMA in action~~ refactor: Put MIMA in action Mar 31, 2023

quintesse reviewed Apr 3, 2023

View reviewed changes

cstamas commented May 4, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into mima

072b13b

cstamas added 2 commits November 18, 2023 17:46

Upgrade MIMA to 2.4.2

43a2fdf

Simplify and better express intenty

778b98a

cstamas added 3 commits December 6, 2023 09:56

Merge remote-tracking branch 'origin/main' into mima

2b5e563

Ignore settings.xml discovered repositories, JBang drives this

ff51b8a

Update ot MIMA 2.4.3

bd5add1

It picks up latest Resolver 1.9.18 that causes less surprises to Windows users.

maxandersen reviewed Dec 7, 2023

View reviewed changes

maxandersen approved these changes Dec 7, 2023

View reviewed changes

maxandersen merged commit 443bcce into jbangdev:main Dec 7, 2023
11 checks passed

allcontributors bot mentioned this pull request Dec 7, 2023

add cstamas as a contributor for code #1710

Merged

cstamas deleted the mima branch December 9, 2023 13:17

refactor: Put MIMA in action #1604

refactor: Put MIMA in action #1604

Conversation

cstamas commented Mar 31, 2023 • edited

maxandersen commented Mar 31, 2023

cstamas commented Mar 31, 2023 • edited

quintesse commented Mar 31, 2023

cstamas commented Mar 31, 2023 • edited

cstamas commented Mar 31, 2023

quintesse commented Mar 31, 2023

cstamas commented Mar 31, 2023 • edited

cstamas commented Mar 31, 2023

quintesse commented Mar 31, 2023

quintesse commented Mar 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas commented Apr 28, 2023

quintesse commented Apr 28, 2023

cstamas commented Apr 28, 2023 • edited

quintesse commented May 4, 2023

cstamas commented May 4, 2023 • edited

cstamas commented May 4, 2023

quintesse commented May 4, 2023

cstamas commented May 4, 2023

quintesse commented May 4, 2023

cstamas commented May 4, 2023

cstamas commented May 4, 2023

quintesse commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quintesse commented May 5, 2023

quintesse commented May 5, 2023

cstamas commented May 5, 2023

cstamas commented May 5, 2023

maxandersen commented Nov 18, 2023

maxandersen commented Nov 18, 2023

cstamas commented Nov 18, 2023

cstamas commented Nov 18, 2023 • edited

cstamas commented Nov 18, 2023

maxandersen commented Nov 19, 2023

maxandersen commented Nov 19, 2023

cstamas commented Nov 19, 2023

cstamas commented Nov 19, 2023 • edited

maxandersen commented Nov 19, 2023

cstamas commented Nov 19, 2023

maxandersen commented Nov 19, 2023

maxandersen commented Dec 6, 2023

cstamas commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxandersen left a comment

Choose a reason for hiding this comment

maxandersen commented Dec 7, 2023

allcontributors bot commented Dec 7, 2023

quintesse commented Dec 7, 2023

cstamas commented Mar 31, 2023 •

edited

cstamas commented Mar 31, 2023 •

edited

cstamas commented Mar 31, 2023 •

edited

cstamas commented Mar 31, 2023 •

edited

cstamas commented Apr 28, 2023 •

edited

cstamas commented May 4, 2023 •

edited

cstamas commented Nov 18, 2023 •

edited

cstamas commented Nov 19, 2023 •

edited