[MNG-6869] New flag to verify Maven installation status #995
Conversation
|
Isnt it something a plugin would be a better location? Can need some basic downloads but would enable to do more checks on the project, better configure the dummy artifact - 3.8.6 or maven cant be hardcoded in case of central "mirroring", it can be forbidden intentionally. |
maven-embedder/src/main/java/org/apache/maven/cli/MavenStatusCommand.java
Outdated
Show resolved
Hide resolved
maven-embedder/src/main/java/org/apache/maven/cli/MavenStatusCommand.java
Outdated
Show resolved
Hide resolved
maven-embedder/src/main/java/org/apache/maven/cli/MavenStatusCommand.java
Show resolved
Hide resolved
maven-embedder/src/main/java/org/apache/maven/cli/RemoteRepositoryConnectionVerifier.java
Outdated
Show resolved
Hide resolved
maven-embedder/src/main/java/org/apache/maven/cli/RemoteRepositoryConnectionVerifier.java
Outdated
Show resolved
Hide resolved
maven-embedder/src/main/java/org/apache/maven/cli/RemoteRepositoryConnectionVerifier.java
Outdated
Show resolved
Hide resolved
| return issues; | ||
| } | ||
|
|
||
| private List<String> verifyArtifactResolution(final MavenExecutionRequest mavenExecutionRequest) { |
There was a problem hiding this comment.
I think this is 1 step too much. Is this the only way to verify if a remote repository is accessible? If you need to use an explicit file instead of a directory, is it possible to use http HEAD ?
There was a problem hiding this comment.
[...] 1 step too much [...]
Just to verify: are you referring to the fact that this code actually downloads a file (e.g., it could've been an HTTP HEAD request), or to the fact that the --status verifies if artifact resolution works?
In the case of the former: is a remote repository required to support an HTTP HEAD request? Could we rely on the fact that if HTTP HEAD is OK, HTTP GET will be OK, too?
There was a problem hiding this comment.
It depends on what you define as "artifact resolution works". I'm not interested if the implementation is correct. I wish there's a way to only confirm that the connection works. It is about returning questions on StackOverflow that say Maven can't download things, but it is just that it cannot reach a repository. Possible rootcauses are missing proxies (or misconfigured) and misconfigured mirrors. Maybe @michael-o or @cstamas can think of a low-level reliable way to conform this.
There was a problem hiding this comment.
From my experience this is not testing a random artifact which works but more enabling the aether logs which help, ultimately logging the request to be able to replay it with curl to do the check. Rest is random tests which can easily be proven false positive/negative due to proxies (correct) configuration.
There was a problem hiding this comment.
The goal of this code is not to explain what is wrong and how the user should fix it. I agree with @rmannibucau that the Aether logs are more suitable for that. What this code wants to do is tell the user they have a problem, without even building a project.
Ideally, in the case of the StackOverflow questions that @rfscholte refers to, we could say "please share the output of mvn --status". Their output might look like this for example:
[INFO] Local repository setup check completed
[INFO] Connection check for repository 'corporate-repo' at 'https://pkgs.dev.azure.com/big-corporate/project/_packaging/whatever-repository/maven/v1' completed
Downloading from corporate-repo: https://pkgs.dev.azure.com/big-corporate/project/_packaging/whatever-repository/maven/v1/org/apache/maven/apache-maven/3.8.6/apache-maven-3.8.6.pom
Downloading from misconfigured-mirror: https://non-existing.example.com/maven2/org/apache/maven/apache-maven/3.8.6/apache-maven-3.8.6.pom
[INFO] Artifact resolution check completed
[INFO]
[ERROR] The following issues where found
[ERROR] 1. Connection to misconfigured-mirror [https://non-existing.example.com/maven2/] not possible. Cause: non-existing.example.com: nodename nor servname provided, or not known
[ERROR] 2. Could not find artifact org.apache.maven:apache-maven:pom:3.8.6 in corporate-repo (https://pkgs.dev.azure.com/big-corporate/project/_packaging/whatever-repository/maven/v1)
[ERROR] 3. Could not transfer artifact org.apache.maven:apache-maven:pom:3.8.6 from/to misconfigured-mirror (https://non-existing.example.com/maven2/): non-existing.example.com: nodename nor servname provided, or not known
Even though this doesn't tell the user the solution, it points them in a few directions where they could look. It's a lot more compact than the complete Aether logs - which the user could still inspect, in case this message doesn't point them in the right direction already.
The current approach does not need any additional download. I think that's a good thing, as we can now use it to also point the user at issues in their configuration that would prevent artifact resolution. If we moved this to a plugin, we'd need artifact resolution to actually work...
I'm open to suggestions for another publicly available artifact. But in order to "prove" that artifact resolution works, we need something that is publicly available - I think something that at the very least is hosted by Central, and maybe by mirrors.
More thorough checks could reside in a plugin, but as pointed out before, we can't rely on resolution to work until we've checked that it works. So agreed, additional checks could live in a plugin, but I'm convinced that we'll need a few basic checks (among them artifact resolution and local repository setup) to live in Core. |
Point is none can be hardcoded, cause we dont know companies mirrors. If you dont like the plugin, what about doing a |
|
Why not move it into a module within Maven Core which could be no-op of this default code. It could be easily exchanged?! |
|
@michael-o or just enhance the CLI with a small SPI to enable to add custom command with extensions so this does not need a noop at all and is purely optional and in a dedicated module which can get a real configuration and enhancements later? |
|
Personally, I don't see much value in moving the code to a new module. If I understand correctly, we'd be moving this code to a different module inside the same Git repository, only to introduce some complexity in maven-embedder to load that exact same code. Also, this PR doesn't pull in new dependencies to maven-embedder, which we could have avoided by moving the code elsewhere, so I really fail to see the benefits of such a refactoring. |
This could happen when the artifact resolution did not work due to other reasons than a non-existing artifact - such as a non-existing hostname.
mthmulders
force-pushed
the
mng-6869-new-flag-to-verify-maven-status
branch
from
45ba62e
to
853819c
Compare
|
That would completely defy the utility and value of this feature. I do want a clean Maven installation to be able to do a few basic checks. That is exactly the idea as laid out in MNG-6869, if you ask me. Anything that requires a separate download (a plugin, or a helper script, or extension, or ....) and we do not ship with Apache Maven itself is completely opposite to the original idea, and would therefore be a
I'm sorry, I don't understand what you mean here.
If it were to become much bigger, we could always move it out later. As long as we can ship it with Apache Maven by default (see point 1). Edit: I am aware of ${maven.home}/lib/ext/, but as far as I can see, this is only used for 3rd-party libraries - not for stuff that we maintain ourselves. |
| @@ -30,11 +31,11 @@ | |||
| * A wrapper class around a maven resolver artifact. | |||
| */ | |||
| public class DefaultArtifactCoordinate implements ArtifactCoordinate { | |||
| private final @Nonnull AbstractSession session; | |||
There was a problem hiding this comment.
Can this change be removed from this PR ? It's completely unrelated.
Also, if we do change that, other classes such as DefaultArtifact and maybe others use the same mechanism and should be changed at the same time.
There was a problem hiding this comment.
Sure! I've just pushed a commit to undo this part.
This pull request adds a few basic checks that users can run to see if they've installed Maven correctly. This includes:
org.apache.maven:apache-maven:3.8.6).These checks honour settings.xml profile activation through
<activeProfiles>. The artifact resolution check honours any configured mirrors.The connection check is quite rudimentary, and only functions at the HTTP connection level. Since there is no guarantee that a repository will contain a particular artifact, there's not much more we can do.
The artifact resolution check is a bit more thorough: it checks if there is any way in which Maven can resolve itself. It does so by temporarily using a "dummy" local repository. This is necessary to prevent a "false positive", where the artifact may have existed before.
We have not added unit tests or integration tests (yet). At some level, they may be useful, but we expect a full integration test to be a significant amount of work that would not provide a lot of value. We're curious to hear your opinions about that trade-off.
Following this checklist to help us incorporate your contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
--> MNG-6869
[MNG-XXX] SUMMARY,where you replace
MNG-XXXandSUMMARYwith the appropriate JIRA issue.[MNG-XXX] SUMMARY.Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.