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-4840] document requiredMavenVersion in plugin descriptor #1444

Merged
merged 1 commit into from Mar 16, 2024

Conversation

hboutemy
Copy link
Member

@hboutemy hboutemy merged commit a137cc6 into maven-3.9.x Mar 16, 2024
26 of 30 checks passed
@hboutemy hboutemy deleted the MNG-4840-documentation branch March 16, 2024 15:57
@kwin
Copy link
Member

kwin commented Mar 21, 2024

@hboutemy I am confused by this PR. MNG-4840 iss not about plugin descriptors but about POMs. The required Maven version in plugin descriptors has been added in https://issues.apache.org/jira/browse/MNG-7570 just recently and is only evaluated in Maven4+.

@hboutemy
Copy link
Member Author

hboutemy commented Mar 25, 2024

I see you added the field to .mdo in https://github.com/apache/maven/pull/832/files#diff-62346c3118817c527a41027bab54349b463add4c91df98f95984012304378c87

the fact is that mdo was not used to generate code, but only to document XML against existing hand-written PluginDescriptor class https://github.com/apache/maven/blob/maven-3.9.x/maven-plugin-api/src/main/java/org/apache/maven/plugin/descriptor/PluginDescriptor.java
= something that IIUC changed in Maven 4 somewhere during the alphas

then the missing field in 3.9 .mdo was "just" a documentation bug that I fixed in this PR: now I understand that it became a missing field in code in Maven 4, and you already added it in #832 :)

@kwin
Copy link
Member

kwin commented Mar 25, 2024

As you can see in

public PluginDescriptor build(Reader reader, String source) throws PlexusConfigurationException {
PlexusConfiguration c = buildConfiguration(reader);
PluginDescriptor pluginDescriptor = new PluginDescriptor();
pluginDescriptor.setSource(source);
pluginDescriptor.setGroupId(c.getChild("groupId").getValue());
pluginDescriptor.setArtifactId(c.getChild("artifactId").getValue());
pluginDescriptor.setVersion(c.getChild("version").getValue());
pluginDescriptor.setGoalPrefix(c.getChild("goalPrefix").getValue());
pluginDescriptor.setName(c.getChild("name").getValue());
pluginDescriptor.setDescription(c.getChild("description").getValue());
String isolatedRealm = c.getChild("isolatedRealm").getValue();
if (isolatedRealm != null) {
pluginDescriptor.setIsolatedRealm(Boolean.parseBoolean(isolatedRealm));
}
String inheritedByDefault = c.getChild("inheritedByDefault").getValue();
if (inheritedByDefault != null) {
pluginDescriptor.setInheritedByDefault(Boolean.parseBoolean(inheritedByDefault));
}
// ----------------------------------------------------------------------
// Components
// ----------------------------------------------------------------------
PlexusConfiguration[] mojoConfigurations = c.getChild("mojos").getChildren("mojo");
for (PlexusConfiguration component : mojoConfigurations) {
MojoDescriptor mojoDescriptor = buildComponentDescriptor(component, pluginDescriptor);
pluginDescriptor.addMojo(mojoDescriptor);
}
// ----------------------------------------------------------------------
// Dependencies
// ----------------------------------------------------------------------
PlexusConfiguration[] dependencyConfigurations =
c.getChild("dependencies").getChildren("dependency");
List<ComponentDependency> dependencies = new ArrayList<>();
for (PlexusConfiguration d : dependencyConfigurations) {
ComponentDependency cd = new ComponentDependency();
cd.setArtifactId(d.getChild("artifactId").getValue());
cd.setGroupId(d.getChild("groupId").getValue());
cd.setType(d.getChild("type").getValue());
cd.setVersion(d.getChild("version").getValue());
dependencies.add(cd);
}
pluginDescriptor.setDependencies(dependencies);
return pluginDescriptor;
}
this field was never evaluated by the parser (although already contained in the PluginDescriptor object). Therefore effectively prior to Maven 4 it is only ever set via
descriptor.setRequiredMavenVersion(artifact.getProperty("requiredMavenVersion", null));
through the according POM field.
Therefore I would keep documenting that this is only supported with Plugin Descriptor v1.1 and Maven 4 only. Prior it was just a shortcut for accessing the plugin's POM prerequisites element.

@hboutemy
Copy link
Member Author

history is complex: when the fiels has been introduced in 3.0.2, it has been filled 40fb188#diff-8a738ae425b439d6a5288b118ce793df13ee73db9cfcb183f7b41633e72abad9L141

not the place where I'd expect it, but used
I don't know if this is still done in Maven 3.9: I'd be interested to see how the change in Maven 4 did manage the lacking field

@hboutemy
Copy link
Member Author

hboutemy commented Mar 25, 2024

still there in Maven 3.9.x https://github.com/apache/maven/blob/maven-3.9.x/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java#L189

oh, now I see https://issues.apache.org/jira/browse/MNG-7570
the field is in plugin descriptor java code since 3.0.2, but not filled from plugin descriptor XML but from plugin artifact's pom.xml!!!

then now I understand that documenting the field in descriptor XML is a mistake
oh, I did not get that, it's something to be clarified in the field javadoc for 3.9.x and probably mdo

@hboutemy
Copy link
Member Author

and re-reading Jira issue description https://issues.apache.org/jira/browse/MNG-7570 "there is currently no element for this on the plugin descriptor", it was true for 4 but not completely for 3.x (field exists in java but not XML)

pfew, what a situation!

@hboutemy
Copy link
Member Author

I'll propose a PR to fix my mistakes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants