bowbahdoe / jresolve

Apache License 2.0
5 stars 2 forks source link

Add support for relocations #2

Open Thihup opened 1 year ago

Thihup commented 1 year ago

One example of dependency that uses relocation is JBoss Jandex that relocated to SmallRye Jandex

https://repo1.maven.org/maven2/org/jboss/jandex/3.1.2/

bowbahdoe commented 1 year ago

So just checking my understanding here. If a POM declares a relocation, that means that we should look at that other artifact to get the actual contents?

Existing tools give some strange behavior around this

➜  ~ cs resolve org.jboss:jandex:3.1.2
https://repo1.maven.org/maven2/org/jboss/jandex/3.1.2/jandex-3.1.2.pom
  100.0% [##########] 777B (1.8 KiB / s)
https://repo1.maven.org/maven2/org/jboss/jboss-parent/12/jboss-parent-12.pom
  100.0% [##########] 30.8 KiB (513.6 KiB / s)
https://repo1.maven.org/maven2/io/smallrye/jandex/3.1.2/jandex-3.1.2.pom
  100.0% [##########] 6.8 KiB (128.4 KiB / s)
https://repo1.maven.org/maven2/io/smallrye/jandex-parent/3.1.2/jandex-parent-3.1.2.pom
  100.0% [##########] 7.0 KiB (131.8 KiB / s)
https://repo1.maven.org/maven2/io/smallrye/smallrye-build-parent/39/smallrye-build-parent-39.pom
  100.0% [##########] 27.7 KiB (542.2 KiB / s)
io.smallrye:jandex:3.1.2:default
org.jboss:jandex:3.1.2:default
➜  ~ cs fetch org.jboss:jandex:3.1.2
/Users/emccue/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/io/smallrye/jandex/3.1.2/jandex-3.1.2.jar
➜  ~ clojure -Sdeps '{:deps {org.jboss/jandex {:mvn/version "3.1.2"}}}' -Stree
Error building classpath. Could not find artifact org.jboss:jandex:jar:3.1.2 in central (https://repo1.maven.org/maven2/)

So in the first case coursier seems to be just taking both artifacts in its resolution, but does end up with the relocated version when fetching. Clojure just breaks (which is interesting since it is using maven resolver under the hood).

My immediate question is how this should affect resolution. Right now resolution works off a map of Library -> Coordinate. I could say that the coordinate should take the relocation rules into account - thats not out of the question - but it feels like its gonna change the conceptual model if it updates the Library...

Open to ideas. I asked in the clojurians slack here if you want to follow my fact finding.

bowbahdoe commented 1 year ago

Tracking issue in TDEPS - https://clojure.atlassian.net/browse/TDEPS-8

bowbahdoe commented 1 year ago

Thinking out loud a bit


public interface Coordinate {
    default Coordinate normalize(Library library, Cache cache) {
        return this;
    }

    enum VersionOrdering {
        INCOMPARABLE,
        GREATER_THAN,
        LESS_THAN,
        EQUAL_TO;

        public static VersionOrdering fromInt(int comparisonResult) {
            return comparisonResult == 0
                    ? EQUAL_TO
                    : comparisonResult > 0
                    ? GREATER_THAN
                    : LESS_THAN;
        }
    }

    VersionOrdering compareVersions(Coordinate coordinate);

    CoordinateId id();

    Manifest getManifest(Cache cache);

    Path getLibraryLocation(Cache cache);

    default Optional<Path> getLibrarySourcesLocation(Cache cache) {
        return Optional.empty();
    }

    default Optional<Path> getLibraryDocumentationLocation(Cache cache) {
        return Optional.empty();
    }
}

I want to preserve a few properties

  1. Library should be a separate concept from Coordinate
  2. All maven specific logic should live in MavenCoordinate.

If we look at the existing methods here, normalize seems like it would engage at the right time in resolution to make corrections. Maybe normalize could return a new Coordinate and a new Library potentially?

The logic of which could be keyed on "the library you are looking for matches up with what i am looking for in the coordinate". That way if someone was looking for org.jboss:jandex we can relocate, but someone else can use the same coordinate at the top level for a different Library.

(The Library in the parameters list is leftover from when I less certain what the role of coordinate would be. Since i haven't done version ranges yet it hasn't been touched. Good time for that!)

bowbahdoe commented 1 year ago

I went on a long drive yesterday and I'm thinking there might be a different course of action to take here - still fail on relocated dependencies, but provide hints as to how to resolve the issue.

addDependencyOverride could be made to take a Library and a Dependency - so it would see org.jboss:jandex and replace it with io.smallrye:jandex and a Coordinate pointed to the right place. This wouldn't make it work automatically, but if we can bubble up enough information to a user to give clear guidance on that it could be enough.