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

Add m2e support #1414

Merged
merged 7 commits into from
Dec 15, 2022
Merged

Add m2e support #1414

merged 7 commits into from
Dec 15, 2022

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Dec 1, 2022

This closes #1413

Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.

Please make sure that your PR allows edits from maintainers. Sometimes its faster for us to just fix something than it is to describe how to fix it.

Allow edits from maintainers

After creating the PR, please add a commit that adds a bullet-point under the [Unreleased] section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:

  • a summary of the change
  • either
    • a link to the issue you are resolving (for small changes)
    • a link to the PR you just created (for big changes likely to have discussion)

If your change only affects a build plugin, and not the lib, then you only need to update the plugin-foo/CHANGES.md for that plugin.

If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update CHANGES.md for both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.

This makes it easier for the maintainers to quickly release your changes :)

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Thanks, great PR! I've got one minor nit below, address that and then I'd be happy to merge. @lutovich do you want to take a look? No worries if you're too busy, if I don't hear from you in a week I'll assume you're okay with it.


@Override
public boolean isUpToDate(Path file) {
return !buildContext.hasDelta(file.toFile()) || delegate.isUpToDate(file);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be && instead of ||?

Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

LGTM! I added one nit suggestion and have two questions just to better understand the change (I have no experience with m2e):

  1. Is my understanding correct that a simple non-Eclipse maven build will inject a DefaultBuildContext that will not affect up-to-date checking?
  2. What BuildContext will be injected when a build happens in Eclipse?

@lutovich
Copy link
Contributor

lutovich commented Dec 4, 2022

Looks like there's a compilation error:

> Task :plugin-maven:compileJava FAILED
/home/circleci/project/plugin-maven/src/main/java/com/diffplug/spotless/maven/incremental/UpToDateChecker.java:52: error: no suitable method found for hasDelta(Path)
				if (buildContext.hasDelta(file)) {
				                ^
    method BuildContext.hasDelta(String) is not applicable
      (argument mismatch; Path cannot be converted to String)
    method BuildContext.hasDelta(File) is not applicable
      (argument mismatch; Path cannot be converted to File)
    method BuildContext.hasDelta(List) is not applicable
      (argument mismatch; Path cannot be converted to List)
1 error

@kwin
Copy link
Contributor Author

kwin commented Dec 5, 2022

LGTM! I added one nit suggestion and have two questions just to better understand the change (I have no experience with m2e):

  1. Is my understanding correct that a simple non-Eclipse maven build will inject a DefaultBuildContext that will not affect up-to-date checking?

Yes, compare also with https://github.com/codehaus-plexus/plexus-build-api/blob/master/README.md#default-implementation

  1. What BuildContext will be injected when a build happens in Eclipse?

The one from https://github.com/eclipse-m2e/m2e-core/blob/d4c976bb9c8d0aabb029030c3c4f3430af8a8c9e/org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/builder/plexusbuildapi/EclipseBuildContext.java

@nedtwigg nedtwigg merged commit 3853efb into diffplug:main Dec 15, 2022
nedtwigg added a commit that referenced this pull request Dec 31, 2022
@nedtwigg
Copy link
Member

nedtwigg commented Jan 2, 2023

Released in plugin-maven 2.29.0.

@kwin kwin deleted the feature/m2e-support branch January 2, 2023 05:52
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.

Support for execution during incremental builds with m2eclipse
3 participants