gnodet commented on code in PR #168: URL: https://github.com/apache/maven-resolver/pull/168#discussion_r857378877
########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/LocalPathPrefixComposerFactorySupport.java: ########## @@ -0,0 +1,177 @@ +package org.eclipse.aether.internal.impl; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.metadata.Metadata; +import org.eclipse.aether.util.ConfigUtils; + +/** + * Support class for {@link LocalPathPrefixComposerFactory} implementations: it predefines and makes re-usable + * common configuration getters, and defines a support class for {@link LocalPathPrefixComposer} carrying same. + * + * @since TBD + */ +public abstract class LocalPathPrefixComposerFactorySupport implements LocalPathPrefixComposerFactory +{ + private static final String CONF_PROP_SPLIT = "aether.enhancedLocalRepository.split"; Review Comment: Those properties should be public given they seem to be of some interest to the user ? ########## src/site/markdown/configuration.md: ########## @@ -47,6 +47,15 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix `aether.dependencyCollector.impl` | String | The name of the dependency collector implementation to use: depth-first (original) named `df`, and breadth-first (new in 1.8.0) named `bf`. Both collectors produce equivalent results, but they may differ performance wise, depending on project being applied to. Our experience shows that existing `df` is well suited for smaller to medium size projects, while `bf` may perform better on huge projects with many dependencies. Experiment (and come back to us!) to figure out which one suits you the better. | `"df"` | no `aether.dependencyCollector.bf.skipper` | boolean | Flag controlling whether to skip resolving duplicate/conflicting nodes during the breadth-first (`bf`) dependency collection process. | `true` | no `aether.dependencyManager.verbose` | boolean | Flag controlling the verbose mode for dependency management. If enabled, the original attributes of a dependency before its update due to dependency managemnent will be recorded in the node's `DependencyNode#getData()` when building a dependency graph. | `false` | no +`aether.enhancedLocalRepository.localPrefix` | String | The prefix to use for locally installed artifacts. | `"installed"` | no +`aether.enhancedLocalRepository.snapshotsPrefix` | String | The prefix to use for snapshot artifacts. | `"snapshots"` | no +`aether.enhancedLocalRepository.split` | boolean | Whether LRM should split local and remote artifacts. | `false` | no +`aether.enhancedLocalRepository.splitLocal` | boolean | Whether locally installed artifacts should be split by version (release/snapshot). | `false` | no +`aether.enhancedLocalRepository.splitRemote` | boolean | Whether cached artifacts should be split by version (release/snapshot). | `false` | no +`aether.enhancedLocalRepository.splitRemoteRepository` | boolean | Whether cached artifacts should be split by origin repository (repository ID). | `false` | no +`aether.enhancedLocalRepository.splitRemoteRepositoryLast` | boolean | For cached artifacts, if both `splitRemote` and `splitRemoteRepository` are set to `true` sets the splitting order: by default it is repositoryId/version (false) or version/repositoryId (true) | `false` | no +`aether.enhancedLocalRepository.remotePrefix` | String | The prefix to use for downloaded and cached artifacts. | `"cached"` | no +`aether.enhancedLocalRepository.releasesPrefix` | String | The prefix to use for release artifacts. | `"releases"` | no Review Comment: This is slightly out of scope for this PR, but while we're at it, I think it would make sense to give some hints on how/where to actually define those properties, or at least maybe point to maven, which is definitely the main consumer of the resolver. ########## src/site/markdown/local-repository.md: ########## @@ -0,0 +1,156 @@ +# Local Repository +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +Maven Resolver implements a "local repository" (that is used by Maven itself +as well), that since the beginning of time was a "mixed bag of beans", +it served twofold purposes: to cache the artifacts downloaded from +remote, but also to store the artifacts locally installed (locally built and +installed, to be more precise). Both of these artifacts were stored in bulk +in the local repository. + +Local repository implementations implement the `LocalRepositoryManager` (LRM) +interface, and Resolver out of the box provides two implementations for it: +"simple" and "enhanced". + +## Simple LRM + +Simple is a fully functional LRM implementation, but is used +mainly in tests, it is not recommended in production environments. + +To manually instantiate a simple LRM, one needs to invoke following code: + +```java +LocalRepositoryManager simple = new SimpleLocalRepositoryManagerFactory() + .newInstance( session, new LocalRepository( baseDir ) ); +``` + +Note: This code snippet above instantiates a component, that is not +recommended way to use it, as it should be rather injected whenever possible. +This example above is merely a showcase how to obtain LRM implementation +in unit tests. + +## Enhanced LRM + +Enhanced LRM on the other hand is enhanced with several extra +features, one most notable is scoping cached content by its origin and context: +if you downloaded an artifact A1 from repository R1 +and later initiate build that requires same artifact A1, but repository R1 +is not defined in build, the A1 artifact cached from R1 remote repository should be handled +as not present, and needs to be re-downloaded, despite same coordinates. +Those two, originating from two different repositories may not be the same thing. +This is meant to protect users from "bad practice" (artifact coordinates are +unique in ideal world). + +### Split Local Repository + +Latest addition to the enhanced LRM is *split* feature. By default, split +feature is **not enabled**, enhanced LRM behaves as it behaved in all +previous versions of Resolver. + +Enhanced LRM is able to split the content of local repository by +several conditions: + +* differentiate between *cached* and locally *installed* artifacts +* differentiate *cached* artifacts based on their origin (remote repository) +* differentiate between *release* and *snapshot* versioned artifacts + +The split feature is implemented by the `LocalPathPrefixComposer` interface, +that adds different "prefixes" for the locally stored artifacts, based on +their context. + +#### Note About Release And Snapshot Differentiation + +The prefix composer is able to differentiate between release and snapshot +versioned artifacts, and this is clear-cut: Maven Artifacts are either +this or that. + +On the other hand, in case of Maven Metadata, things are not so clear. +Resolver is able to differentiate *only* based on `metadata/version` +field, but that field is not always present. + +Maven Metadata exists in 3 variants: + +* G level metadata does not carry version. +* GA level metadata does not carry version. +* GAV level metadata carries version. + +In short, G and GA level metadata *always* end up in releases, despite +GA level metadata body may contain mixed release and snapshot versions enlisted, +as this metadata *does not contain* the `metadata/version` field. + +The GAV level metadata gets differentiated based on version it carries, so +they may end up in releases or snapshots, depending on their value of +`metadata/version` field. + +#### Use Cases + +Most direct use case is simpler local repository eviction. One can delete all +locally built artifacts without deleting the cached ones, hence, no +need to re-download the "whole universe". Similarly, deletion of cached ones +can happen based even on origin repository (if split by remote repository +was enabled beforehand). + +Example configuration with split by remote repository: +```java +$ mvn ... -Daether.enhancedLocalRepository.split \ + -Daether.enhancedLocalRepository.splitRemoteRepository +``` + +Another use case is interesting for "branched development". Before split feature, +a developer simultaneously working on several branches of same project was forced +to rebuild all (better: install all), as same built artifacts from different +branches would still land in the local repository on same coordinates, hence, they +would constantly overwrite each other. It was easy to get into false error +state where partially overlapping content were present in the local repository from +different branches. Now one can set unique local prefix for each +branch it is working on (or even by project, like +`-Daether.enhancedLocalRepository.localPrefix=$PROJECT/$BRANCH`, but use +actual values, these expressions are merely an example, there is no interpolation +happening!) and the +local repository becomes usable even simultaneously, even concurrently from +different terminals, as different projects and their branches can simply +coexist in local repository. They will land in different places, due different +prefixes. + +Example configuration for branches: +```java +$ mvn ... -Daether.enhancedLocalRepository.split \ + -Daether.enhancedLocalRepository.localPrefix=maven-resolver/mresolver-253 + -Daether.enhancedLocalRepository.splitRemoteRepository +``` + Review Comment: I think it would make sense to point and link to the `configuration.md` page where all the properties are explained. ########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultLocalPathPrefixComposerFactory.java: ########## @@ -0,0 +1,141 @@ +package org.eclipse.aether.internal.impl; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import javax.inject.Named; +import javax.inject.Singleton; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.metadata.Metadata; +import org.eclipse.aether.repository.RemoteRepository; + +/** + * Default local path prefix composer factory: it creates {@link LocalPathPrefixComposer} instances of (internal) type + * {@link DefaultLocalPathPrefixComposer} that observe and implement all the supported parameters predefined in support + * class. + * + * @since TBD + */ +@Singleton +@Named +public final class DefaultLocalPathPrefixComposerFactory extends LocalPathPrefixComposerFactorySupport Review Comment: Do we have to use `final` / `private` here ? The doc hints at implementing a new strategy if needed, but we don't really have to make users life difficult, do we ? Maybe removing `final` and make the `DefaultLocalPathPrefixComposer` a `protected` class, in case someone needs to modify the behavior without rewriting the whole class ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
