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

[MNG-8510] Rename cling.invoker.Utils class and reduce public API surface #2041

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Jan 11, 2025

None of these methods seem to be tested or documented so the more we hide them the more flexibility we have to modify or change them in the future.

@elharo
Copy link
Contributor Author

elharo commented Jan 11, 2025

Looks like another CI flake:

[main] ERROR org.apache.maven.cling.invoker.PlexusContainerCapsule - Failed to read artifact descriptor for org.apache.maven:maven-archiver:jar:3.6.2: Failed to collect dependencies at org.apache.maven.plugins:maven-jar-plugin:jar:3.4.2 -> org.apache.maven:maven-archiver:jar:3.6.2: Unable to resolve artifact: The following artifacts could not be resolved: org.apache.maven.shared:maven-shared-components:pom:41 (absent): Could not transfer artifact org.apache.maven.shared:maven-shared-components:pom:41 from/to central (https://repo.maven.apache.org/maven2): HTTP connect timed out (remote repositories: central (https://repo.maven.apache.org/maven2, default, releases), apache.snapshots (https://repository.apache.org/snapshots, default, snapshots))
[main] ERROR org.apache.maven.cling.invoker.PlexusContainerCap

@elharo elharo marked this pull request as ready for review January 11, 2025 14:51
@elharo elharo requested a review from gnodet January 11, 2025 14:51
@elharo elharo changed the title [MNG-8510] Rename Utils class and reduce public API surface [MNG-8510] Rename cling.invoker.Utils class and reduce public API surface Jan 11, 2025
@gnodet
Copy link
Contributor

gnodet commented Jan 11, 2025

Looks like another CI flake:

[main] ERROR org.apache.maven.cling.invoker.PlexusContainerCapsule - Failed to read artifact descriptor for org.apache.maven:maven-archiver:jar:3.6.2: Failed to collect dependencies at org.apache.maven.plugins:maven-jar-plugin:jar:3.4.2 -> org.apache.maven:maven-archiver:jar:3.6.2: Unable to resolve artifact: The following artifacts could not be resolved: org.apache.maven.shared:maven-shared-components:pom:41 (absent): Could not transfer artifact org.apache.maven.shared:maven-shared-components:pom:41 from/to central (https://repo.maven.apache.org/maven2): HTTP connect timed out (remote repositories: central (https://repo.maven.apache.org/maven2, default, releases), apache.snapshots (https://repository.apache.org/snapshots, default, snapshots)) [main] ERROR org.apache.maven.cling.invoker.PlexusContainerCap

This is always an http download issue and they only occur on MacOS.
It's an infrastructure issue, @cstamas and I have been trying to fix those by adding GitHub cache to the ITs, but it's not an easy task.

@gnodet gnodet mentioned this pull request Jan 13, 2025

public abstract class BaseParser implements Parser {

@Nullable
private static Path findRoot(Path topDirectory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not at ease with splitting the findRoot and findMandatoryRoot in different classes. Those are related and very close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't share code and each is used in a single different place. Both are implementation details that should be hidden from public view. Neither is directly tested. This all suggests they should be private methods where they're used.

@gnodet
Copy link
Contributor

gnodet commented Jan 14, 2025

@elharo am I wrong or do you consider everything that is publicly accessible part of some sort of API ?
if so, why ? with the new API coming around, the API is defined to be the public parts of maven-api-xxx artifacts.
Everything else is not part of the API and plugins, so if we follow, we should make them all package protected ?
Could we agree on the boundaries of the API ?

@elharo
Copy link
Contributor Author

elharo commented Jan 14, 2025

Generally yes, Hyrum's Law applies: https://www.hyrumslaw.com/

That said, there is a spectrum. public > protected > package protected > private

and if you name a package internal or add an Experimental annotation, it will be less frequently depended on. But overall keep your APIs as small as possible. That's less trouble for users and more flexibility for developers.

@cstamas
Copy link
Member

cstamas commented Jan 14, 2025

You forget one thing: maven4 plugins, users of API will have no access to anything else, like maven-cli is. That is the point, that maven3 was in fact "free for all" as classloader did expose all the maven guts to them. Maven4 w/ maven4 plugins does not expose anything else than API

@elharo
Copy link
Contributor Author

elharo commented Jan 17, 2025

You're assuming they're being used by a Maven 4 plugin following the rules. That's certainly one way they will be used, but they will also be used by devs who add the Maven artifacts to their build file because their web app wants to stripLeadingAndTrailingQuotes from some string somewhere, and this looks like it does it. Developers can and do wire in utility methods that pull in frameworks the size of Spring because they want half-baked, buggy string processing utility. I've seen it before. I'm sure I'll see it again.

But this is not just about developers who use the jars in ways that aren't planned for or intended. Reducing the public API footprint also makes it easier for tools like static analyzers and compilers to reason about the code.

And most importantly, reducing the public API surface makes it easier for humans working on this code base to see what it does in one place. Separating tightly coupled pieces into separate files makes code much harder to read and understand.

@cstamas
Copy link
Member

cstamas commented Jan 17, 2025

Sure, we cannot prevent any (mis)use of these artifacts.

BUT, in my POV, we do owe some promises to the first group (using them "as intended"), but we do not owe any promises for the second group.

The second case parallel could be "is a book publisher/author guilty, if I used their book to stack my shelf with porcelain, but it failed, and I lost my family heritage of expensive porcelain". Yes, books can be used to stack/support things, but that is not their "intended use". For me, your use case do exists, but is not my role to "prevent breaking it".

I always assumed that programmers can read ("what should I do" vs "what can I do"). Documents like this one https://maven.apache.org/resolver/api-compatibility.html explains it. Those "what can I do" are left on their own. The CAN do it, I will not prevent them (making their but our lives more miserable by locking down all into private classes), but they should be aware that they are on their own. Or to rephrase: when I look onto a library, I don't consider a class "part of API" only due it's public modifier. Many things counts, like for example package is in ...impl or ...internal, etc. Is not and was never black or white.

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.

3 participants