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.

Sign up for GitHub

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

Jump to bottom

[MDEP-674] Add IDE build support #257

Merged
merged 6 commits into from Nov 22, 2022
Merged

[MDEP-674] Add IDE build support #257

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c22e5a5
[MDEP-674] Add m2e lifecycle metadata
kwin Oct 28, 2022
c34c3c7
fix comments
kwin Nov 9, 2022
2da2ab8
replace m2e link by plexus-build-api one
kwin Nov 9, 2022
512800e
add more context to why old coordinates are used
kwin Nov 9, 2022
5c310ad
remove m2e specific metadata
kwin Nov 9, 2022
1b79277
reword javadoc
kwin Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions pom.xml
View file
Open in desktop
Expand Up @@ -247,6 +247,15 @@ under the License.
<version>${resolverVersion}</version>
<scope>provided</scope>
</dependency>
<!-- incremental build support (http://www.eclipse.org/m2e/documentation/m2e-making-maven-plugins-compat.html) -->
kwin marked this conversation as resolved.
Show resolved Hide resolved
<dependency>
<groupId>org.sonatype.plexus</groupId>
<artifactId>plexus-build-api</artifactId>
<version>0.0.7</version>
<scope>compile</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

unmaintained dependency ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, it saw a new release at https://github.com/codehaus-plexus/plexus-build-api. The new GAV and packages are just not compatible with m2e yet (eclipse-m2e/m2e-core#944).

Copy link
Member Author

Choose a reason for hiding this comment

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

Compare also with the discussion at https://lists.apache.org/thread/h2gy5l7j5bv7ycbpwdjby66v8gwhzck2.


<!-- test -->
<dependency>
<groupId>org.eclipse.aether</groupId>
<artifactId>aether-connector-basic</artifactId>
Expand All @@ -265,8 +274,6 @@ under the License.
<version>${resolverVersion}</version>
<scope>test</scope>
</dependency>

<!-- test -->
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Expand Down
22 changes: 22 additions & 0 deletions src/main/java/org/apache/maven/plugins/dependency/AbstractDependencyMojo.java
View file
Open in desktop
Expand Up @@ -46,6 +46,7 @@
import org.codehaus.plexus.util.FileUtils;
import org.codehaus.plexus.util.ReflectionUtils;
import org.codehaus.plexus.util.StringUtils;
import org.sonatype.plexus.build.incremental.BuildContext;

/**
* @author <a href="mailto:brianf@apache.org">Brian Fox</a>
Expand All @@ -59,6 +60,21 @@ public abstract class AbstractDependencyMojo
@Component
private ArchiverManager archiverManager;


/**
* For m2e incremental build support
Copy link

Choose a reason for hiding this comment

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

Since the BuildContext API is not Eclipse M2E specific (even though M2E is the only user I'm aware of) I suggest to make this comment more generic.
The same applies for the comment of the skipDuringIncrementalBuild.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is mostly to not confuse it with http://takari.io/2014/03/25/incremental-build.html. Any suggestion for the naming as just incremental build support may be a bit too generic (as there are other incremental build APIs)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe IDE build support as this is also not only about incremental support but also about other features from the Build Support API (like notification about updates files or context-specific error messages)

Copy link

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in c34c3c7.

*/
@Component
private BuildContext buildContext;

/**
* Skip plugin execution during incremental builds (e.g. triggered from M2E).
*
* @since 3.3.1
slawekjaranowski marked this conversation as resolved.
Show resolved Hide resolved
*/
@Parameter( defaultValue = "false" )
private boolean skipDuringIncrementalBuild;

/**
* <p>
* will use the jvm chmod, this is available for user and all level group level will be ignored
Expand Down Expand Up @@ -189,6 +205,7 @@ protected void copyFile( File artifact, File destFile )
}

FileUtils.copyFile( artifact, destFile );
buildContext.refresh( destFile );
}
catch ( IOException e )
{
Expand Down Expand Up @@ -326,6 +343,7 @@ protected void unpack( Artifact artifact, String type, File location, String inc
{
throw new MojoExecutionException( "Error unpacking file: " + file + " to: " + location, e );
}
buildContext.refresh( location );
}

private void silenceUnarchiver( UnArchiver unArchiver )
Expand Down Expand Up @@ -410,6 +428,10 @@ public void setUseJvmChmod( boolean useJvmChmod )
*/
public boolean isSkip()
{
if ( skipDuringIncrementalBuild && buildContext.isIncremental() )
{
return true;
}
return skip;
}

Expand Down
68 changes: 68 additions & 0 deletions src/main/resources/META-INF/m2e/lifecycle-mapping-metadata.xml
View file
Open in desktop
@@ -0,0 +1,68 @@
<!--
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.
-->
<lifecycleMappingMetadata>
Copy link
Member

Choose a reason for hiding this comment

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

Specific only for Eclipse ...

Copy link
Member Author

Choose a reason for hiding this comment

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

could be used by any IDE supporting incremental builds, but currently m2e is the only consumer, right.

Copy link

Choose a reason for hiding this comment

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

Eclipse and its derivative, such as Java edition in VSCode.

Copy link
Member

@olamy olamy Nov 2, 2022 •
edited

Choose a reason for hiding this comment

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

is there option to not store such specific IDE file here?
I don't like much this... I would be -1.
we already have a sort of specific API and now specific files.
Imagine if every IDE ask for storing their specific files?
sounds a bad design to me.

Copy link
Member Author

@kwin kwin Nov 2, 2022 •
edited

Choose a reason for hiding this comment

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

Fact is: There is no other metadata defined for any other IDE. Every IDE could reuse this metadata. We already have m2e metadata in other Maven plugins e.g. in https://github.com/apache/maven-resources-plugin/blob/master/src/main/resources/META-INF/m2e/lifecycle-mapping-metadata.xml or in https://github.com/apache/maven-enforcer/tree/master/maven-enforcer-plugin/src/main/resources/META-INF/m2e.

Copy link
Member

Choose a reason for hiding this comment

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

which should have never been accepted....

Copy link

Choose a reason for hiding this comment

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

Imagine if every IDE ask for storing their specific files? sounds a bad design to me.

Indeed that would be bad and was the reason why I/we the M2E team reached out to the Maven devs on their mailing list (the link @kwin posted above) to establish a unified API defined/maintained by Maven that all IDEs could use. Eventually this probably would have lead to a similar Maven-defined/maintained lifecycle-mapping metadata format (which actually also could be generated from annotations see e.g. eclipse-m2e/m2e-core#830).
But there was only negative feedback from all Maven devs about that suggestion, so each IDE has to continue to handle it their one way.

Anyway, as @mickaelistria said below we can maintain that metadata in a Eclipse M2E plug-in.

Copy link
Member

@cstamas cstamas Nov 2, 2022 •
edited

Choose a reason for hiding this comment

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

Indeed that would be bad and was the reason why I/we the M2E team reached out to the Maven devs on their mailing list (the link @kwin posted above) to establish a unified API defined/maintained by Maven that all IDEs could use. [...] But there was only negative feedback from all Maven devs about that suggestion, so each IDE has to continue to handle it their one way.

A very very strange conclusion drawn from a ML thread having this post: https://lists.apache.org/thread/044tno4lvdjvm4yv7orsqpmn3x4175c4

(edit: typo)

Copy link

Choose a reason for hiding this comment

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

That's right Guillaume Nodet was not averse to the suggestion, but from the other not so positive comments I had the impression that this was not the general conclusion. However, if that was a misinterpretation from my side and the suggestion will be considered in the future, I'm glad. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

As there was no consensus to add m2e specific metadata I removed it now in c34c3c7.

<pluginExecutions>
<pluginExecution>
<pluginExecutionFilter>
<goals>
<goal>copy</goal>
<goal>copy-dependencies</goal>
<goal>properties</goal>
<goal>unpack</goal>
<goal>unpack-dependencies</goal>
</goals>
</pluginExecutionFilter>
<action>
<execute>
<runOnIncremental>true</runOnIncremental>
<runOnConfiguration>false</runOnConfiguration>
</execute>
</action>
</pluginExecution>
<pluginExecution>
<pluginExecutionFilter>
<goals>
<goal>analyze</goal>
<goal>analyze-dep-mgmt</goal>
<goal>analyze-only</goal>
<goal>analyze-report</goal>
<goal>analyze-duplicate</goal>
<goal>build-classpath</goal>
<goal>display-ancestors</goal>
<goal>get</goal>
<goal>go-offline</goal>
<goal>list</goal>
<goal>list-classes</goal>
<goal>list-repositories</goal>
<goal>purge-local-repository</goal>
<goal>resolve</goal>
<goal>resolve-plugins</goal>
<goal>sources</goal>
<goal>tree</goal>
</goals>
</pluginExecutionFilter>
<action>
<execute>
<runOnIncremental>false</runOnIncremental>
<runOnConfiguration>false</runOnConfiguration>
</execute>
kwin marked this conversation as resolved.
Show resolved Hide resolved
</action>
</pluginExecution>
</pluginExecutions>
</lifecycleMappingMetadata>