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

Issue #74: Add display-extension-updates #908

Merged

Conversation

andrzejj0
Copy link
Contributor

First stage of extensions support.

Not supporting core extensions. Only build extensions are supported.

@slawekjaranowski please review

@andrzejj0 andrzejj0 marked this pull request as draft January 24, 2023 23:11
@andrzejj0 andrzejj0 marked this pull request as ready for review January 25, 2023 06:36
Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

Extension can also by defined in .mvn/extensions.xml
https://maven.apache.org/configure.html#mvn-extensions-xml-file

@andrzejj0 andrzejj0 force-pushed the issue-74-extensions-support branch 2 times, most recently from 852bdda to 36a8593 Compare January 25, 2023 19:46
@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Jan 25, 2023

@slawekjaranowski added support for core extensions declared in .mvn/extensions.xml

@andrzejj0 andrzejj0 force-pushed the issue-74-extensions-support branch 3 times, most recently from b8dd20f to 76e0619 Compare January 25, 2023 20:01
* @since 2.15.0
*/
public static Stream<Extension> getCoreExtensions(MavenSession session) throws IOException, XmlPullParserException {
Path extensionsFile = session.getCurrentProject().getBasedir().toPath().resolve(".mvn/extensions.xml");
Copy link
Member

Choose a reason for hiding this comment

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

What with multimdule projects?
getBasedir - returns path to current module - imho it is not Maven project root - should be checked by IT

import java.nio.file.Path;
import java.util.stream.Stream;

import org.apache.maven.cli.internal.extension.model.io.xpp3.CoreExtensionsXpp3Reader;
Copy link
Member

Choose a reason for hiding this comment

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

internal implementation ... can be changed - it is risik to use it

@andrzejj0 andrzejj0 force-pushed the issue-74-extensions-support branch 4 times, most recently from c1580f4 to 5a37c0d Compare February 2, 2023 19:39
Comment on lines +1 to +2
<extensions xmlns="http://maven.apache.org/EXTENSIONS/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/EXTENSIONS/1.0.0 http://maven.apache.org/xsd/core-extensions-1.0.0.xsd">
Copy link
Member

Choose a reason for hiding this comment

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

Core extensions can be defined only in project root - in child module it will be skipped as I know.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a check for dummy-impl so why this file exists?

@andrzejj0 andrzejj0 force-pushed the issue-74-extensions-support branch 2 times, most recently from b3375b4 to a686ce2 Compare February 13, 2023 21:42
@slawekjaranowski
Copy link
Member

@ajarmoniuk please rebase this PR ... now I see in diff changes from master ...

@andrzejj0
Copy link
Contributor Author

@ajarmoniuk please rebase this PR ... now I see in diff changes from master ...

Rebased.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

Ok, I see
you create new module versions-model-core-extensions with contain a copy file core-extensions.mdo from Maven core.

I think that better will be use org.apache.maven.cli.internal.extension.model.io.xpp3.CoreExtensionsXpp3Reader even it is internal

pom.xml Outdated
@@ -126,6 +127,7 @@
<maven.compiler.source>${mojo.java.target}</maven.compiler.source>
<junitBomVersion>5.9.1</junitBomVersion>
<mavenVersion>3.2.5</mavenVersion>
<mavenEmbedderVersion>3.3.9</mavenEmbedderVersion>
Copy link
Member

Choose a reason for hiding this comment

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

It is from Maven Core should have the same version as used Maven api and so on

Comment on lines +1 to +2
<extensions xmlns="http://maven.apache.org/EXTENSIONS/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/EXTENSIONS/1.0.0 http://maven.apache.org/xsd/core-extensions-1.0.0.xsd">
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a check for dummy-impl so why this file exists?

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Feb 18, 2023

Ok, I see you create new module versions-model-core-extensions with contain a copy file core-extensions.mdo from Maven core.

I think that better will be use org.apache.maven.cli.internal.extension.model.io.xpp3.CoreExtensionsXpp3Reader even it is internal

Are you sure?

I asked you before when you requested me to change that:

Ok, so I have a question about this. Maybe I could appeal to the Maven project to make this public, or at least export the mdo?
I could also include the mdo file in the versions plugin. But what about its license then? > It's the ASF -- should we keep it as ASF then?
As an alternative, I could create a parser from scratch, be it manually, or using a bespoke XSD file and generate a parser from it.

So, what do you think should be the course of action here?

So, what should it be? I'll wait.

@andrzejj0
Copy link
Contributor Author

I don't see a check for dummy-impl so why this file exists?

to test if the .mvn/extensions is not processed in the child.

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Feb 18, 2023

@slawekjaranowski and if you need time to decide, feel free to remove this from the upcoming release.

It's not so easy.

Maven Embedder 3.2.5 does not have the Core Extensions parser. Only 3.3.9 and later has it. So we cannot use the one from the same version as Maven.

https://maven.apache.org/ref/3.2.5/maven-embedder/xref/index.html

@slawekjaranowski
Copy link
Member

@slawekjaranowski and if you need time to decide, feel free to remove this from the upcoming release.

It's not so easy.

Maven Embedder 3.2.5 does not have the Core Extensions parser. Only 3.3.9 and later has it. So we cannot use the one from the same version as Maven.

https://maven.apache.org/ref/3.2.5/maven-embedder/xref/index.html

Ok, I must review of it more carefully ... so I will do it again in next week

Maybe some one else want to look also - @slachiewicz ?

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Feb 19, 2023

@slawekjaranowski @slachiewicz

It's not the only case like this in this plugin. Please have a look at e.g. parsing raw pom. Currently it's an ad-hoc parser created from scratch in the plugin, but it actually depends on a document defined and owned by the Maven project. So it's a very similar case.

@andrzejj0
Copy link
Contributor Author

@slawekjaranowski I propose that we keep a version of this PR with the file from the Apache Maven project with its own ASF license and its own Modello version. Until there comes an API from Maven, which we can request or implement.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

I think that we not need new module only for a few classes, we not need xsd for it, it is only for internal purpose.

I would like to put core-extensions.mdo into versions-common where is only used.

pom.xml Outdated Show resolved Hide resolved
<execution>
<id>generate-site-doc</id>
<goals>
<goal>xdoc</goal>
Copy link
Member

Choose a reason for hiding this comment

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

We should not generate xsd for this model - it is internally used for reading extensions.xml

@andrzejj0
Copy link
Contributor Author

I think that we not need new module only for a few classes, we not need xsd for it, it is only for internal purpose.

I would like to put core-extensions.mdo into versions-common where is only used.

Applied.

@andrzejj0 andrzejj0 force-pushed the issue-74-extensions-support branch 2 times, most recently from 950810f to ef5f771 Compare February 28, 2023 17:18
@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Feb 28, 2023

I think that we not need new module only for a few classes, we not need xsd for it, it is only for internal purpose.

I would like to put core-extensions.mdo into versions-common where is only used.

Oh yes, I remember it now. The reason why I created that module was that I couldn't go around javadoc generation errors -- classes were generated.

I think that there is no easy way of hinting javadoc to use the autogenerated classes.

Exit code: 1
/Users/ajarmoniuk/IdeaProjects/versions-maven-plugin-2/versions-common/src/main/java/org/codehaus/mojo/versions/utils/CoreExtensionUtils.java:29: error: cannot find symbol
import org.codehaus.mojo.versions.model.io.xpp3.CoreExtensionsXpp3Reader;
^
symbol: class CoreExtensionsXpp3Reader
location: package org.codehaus.mojo.versions.model.io.xpp3
1 error
Command line was: /Users/ajarmoniuk/Library/Java/JavaVirtualMachines/openjdk-19.0.2/Contents/Home/bin/javadoc @options @packages

Refer to the generated Javadoc files in '/Users/ajarmoniuk/IdeaProjects/versions-maven-plugin-2/versions-common/target/site/apidocs' dir.

@andrzejj0
Copy link
Contributor Author

Deleting the contents of '/home/runner/work/versions/versions'
Initializing the repository
Disabling automatic garbage collection
Setting up auth
Fetching the repository
Removing auth
Error: The process '/usr/bin/git' failed with exit code 128

@slawekjaranowski @slachiewicz could you please restart?

@andrzejj0
Copy link
Contributor Author

@slawekjaranowski is there anything else to be done about this PR?

@slawekjaranowski slawekjaranowski added this to the 2.16.0 milestone May 9, 2023
@slawekjaranowski slawekjaranowski linked an issue May 9, 2023 that may be closed by this pull request
@slawekjaranowski
Copy link
Member

Hi,
I was removed maven-embedder from dependency - we have a copy
I restored license header in files with longer history - we don't know why such license was used ...

Wait for build and we can go.

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

Successfully merging this pull request may close these issues.

Support for reporting new extenions versions
2 participants