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

Fix #1716: Update for Gradle incremental compilation API change in Gradle 4.8 and above #1718

Merged
merged 3 commits into from Jun 13, 2018

Conversation

Projects
None yet
@mszabo-wikia
Contributor

mszabo-wikia commented Jun 6, 2018

Looks like incremental compilation API has changed in 4.8, causing Lombok init to fail:

warning: lombok.javac.apt.LombokProcessor could not be initialized. Lombok will not run during this compilation:
java.lang.ClassCastException: org.gradle.api.internal.tasks.compile.processing.IncrementalFiler cannot be cast to jdk.compiler/com.sun.tools.javac.processing.JavacFiler

This change should get Lombok working again on Gradle 4.8 and above; fixes #1716

CC: @pbi-qfs

@pbi-qfs

This comment has been minimized.

Contributor

pbi-qfs commented Jun 6, 2018

Interesting - we should (if it's not directly a JavacFiler) iterate through all the superclasses recursively until we find the gradle one (or end up at Object) - or, even more general, we check a non-JavacFiler class if it has a field named "delegate" in its class hierarchy, which might be a JavacFiler:

/**
 * This class returns the given filer as a JavacFiler. In case the case that the filer is no
 * JavacFiler (e.g. the Gradle IncrementalFiler), its "delegate" field is used to get the JavacFiler
 * (directly or through a delegate field again)
 */
public JavacFiler getJavacFiler(Object filer) {
	if (filer instanceof JavacFiler) return (JavacFiler) filer;
	
	// try to find a "delegate" field in the object, and use this to check for a JavacFiler
	Class<?> filerClass = filer.getClass();
	while (filerClass != null) {
		try {
			return getJavacFiler(tryGetJavacFilerDelegate(filerSuperClass, filer));
		} catch (final Exception e) {
			// delegate field was not found, try on superclass 
		}
		filerClass = filerClass.getSuperclass();
	}

	processingEnv.getMessager().printMessage(Kind.WARNING,
		"Can't get a JavacFiler from " + filer.getClass().getName() + ". Lombok won't work.");
	return null;
}

private Object tryGetJavacFilerDelegate(final Class<?> filerDelegateClass, final Object instance) throws Exception {
	Field field = filerDelegateClass.getDeclaredField("delegate");
	field.setAccessible(true);
	return field.get(instance);
}

We should use the same logic then for the JavacProcessingEnvironment as well. What do you think?

@mszabo-wikia

This comment has been minimized.

Contributor

mszabo-wikia commented Jun 6, 2018

Oh yeah that's a way more robust solution, thanks! I'll update this PR :)

@pbi-qfs

This comment has been minimized.

Contributor

pbi-qfs commented Jun 7, 2018

Thanks, @mszabo-wikia!

There is a copy of getJavacProcessingEnvironment in AnnotationProcessor with different error reporting, which we should adapt as well.

@victorwss

This comment has been minimized.

Contributor

victorwss commented Jun 7, 2018

Yesterday, I was trying to fix this same problem, unaware that you already had a PR for that. After I coded it, I noticed your PR.

Anyway, you can see it here.

This is my code so far:

	private static final String GRADLE_INCREMENTAL_FILER_CLASS_NAME = "org.gradle.api.internal.tasks.compile.processing.IncrementalFiler";


	/**
	 * This class casts the given filer to a JavacFiler. In case of
	 * gradle incremental compilation, the delegate Filer of the gradle wrapper is returned.
	 */
	public JavacFiler getJavacFiler(Filer filer) {
		final Class<?> filerClass = filer.getClass();
		final Class<?> filerSuperClass = filerClass.getSuperclass();
		final Class<?> gradleFilerClass =
				filerClass.getName().equals(GRADLE_INCREMENTAL_FILER_CLASS_NAME) ? filerClass
				: filerSuperClass.getName().equals(GRADLE_INCREMENTAL_FILER_CLASS_NAME) ? filerSuperClass
				: null;
		if (gradleFilerClass != null) {
			try {
				Field field = gradleFilerClass.getDeclaredField("delegate");
				field.setAccessible(true);
				Object delegate = field.get(filer);
				return (JavacFiler) delegate;
			} catch (final Exception e) {
				e.printStackTrace();
				processingEnv.getMessager().printMessage(Kind.WARNING,
						"Can't get the delegate of the gradle IncrementalFiler. Lombok won't work.");
			}
		}
		if (!(filer instanceof JavacFiler)) {
			String message = "Don't know how to handle unknown filer " + filerClass.getName() + ". Lombok won't work.";
			processingEnv.getMessager().printMessage(Kind.WARNING, message);
			throw new RuntimeException(message);
		}
		return (JavacFiler) filer;
	}
@mszabo-wikia

This comment has been minimized.

Contributor

mszabo-wikia commented Jun 7, 2018

Thanks @pbi-qfs I've updated the logic there as well. I assume the two implementations need to remain separate, right?

@mszabo-wikia

This comment has been minimized.

Contributor

mszabo-wikia commented Jun 7, 2018

@victorwss Sorry about that. I did update the linked issue once I was ready with the PR so something must've gone wrong in the communication.

@pbi-qfs

This comment has been minimized.

Contributor

pbi-qfs commented Jun 7, 2018

@mszabo-wikia Great, lokks fine. It's kind of difficult to unify the two getters since they slightly differ in what they return - if you have a good way to do, step forward. It would ease future maintainability, since you wouldn't forget one place when fixing another.

Maybe extract a common "resolveDelegate(object,targetClassName)" method which we call in all thrww places?

@mszabo-wikia

This comment has been minimized.

Contributor

mszabo-wikia commented Jun 11, 2018

hmm, sounds like a good idea. however I think such refactor should be a separate change so as to avoid making too many changes as part of this PR

@pbi-qfs

This comment has been minimized.

Contributor

pbi-qfs commented Jun 11, 2018

Ok, then from my Point of view, the code is good to merge. @rzwitserloot, what do you think?

@rzwitserloot rzwitserloot merged commit 6311a7e into rzwitserloot:master Jun 13, 2018

@rzwitserloot

This comment has been minimized.

Owner

rzwitserloot commented Jun 13, 2018

Looks good! Merged. @mszabo-wikia I noticed your name is not yet in our AUTHORS file. If you file another PR where you add your name between Mart and Mateusz, we can make it official. Thanks!

@mszabo-wikia mszabo-wikia deleted the Wikia:gradle-incremental-fix branch Jun 13, 2018

@mszabo-wikia

This comment has been minimized.

Contributor

mszabo-wikia commented Jun 20, 2018

@rzwitserloot sweet, thanks, I'll update that :)

@oehme

This comment has been minimized.

oehme commented Jul 12, 2018

@rzwitserloot Could you push a release with this change? I'd like to add a smoke test to Gradle so we can proactively reach out if we ever break stuff again. Also I want to add a performance test.

@rzwitserloot

This comment has been minimized.

Owner

rzwitserloot commented Jul 17, 2018

@oehme The current edge release includes this. Do you need a full release that's also on maven-central, or will an edge release do? These can only be downloaded from https://projectlombok.org/download-edge

@dmaslakov

This comment has been minimized.

dmaslakov commented Jul 18, 2018

@rzwitserloot I have faced this issue and it stops me from using Gradle 4.9. I would prefer the full release as easier to use. Thanks in advance.

@bwolff

This comment has been minimized.

bwolff commented Jul 18, 2018

@rzwitserloot I agree with @dmaslakov, it would be very helpful if there was a minor bugfix release that makes it compatible with Gradle 4.8/4.9, if it's not too much of a hassle. Thank you very much!

@oehme

This comment has been minimized.

oehme commented Jul 18, 2018

@rzwitserloot I personally could work with the edge release, but I think our users would very much prefer a full one.

@OdysseusLives

This comment has been minimized.

OdysseusLives commented Jul 19, 2018

+1, I would prefer a full release to use this

@rspilker

This comment has been minimized.

Collaborator

rspilker commented Jul 20, 2018

We will do a release soon, but are waiting for enough people to have tried the edge release. Especially the changes for #1359 might break something.

@aplatypus

This comment has been minimized.

aplatypus commented Jul 30, 2018

After updating the Lombok jar to: 1.18.2 (latest), I found a further problem if even for a file that doesn't use Lombok compiled under a groovy folder tree name with Gradle v4.9.

Still works fine with Gradle 4.8.1 and earlier.

@aplatypus

This comment has been minimized.

aplatypus commented Jul 31, 2018

Hi, I was refered back to this issue regarding a Different Build problem. I am unclear of the status now.

I had a message that there was a Lombok release, v1.18.2, and I thought that meant that v1.18.2 ought to address this bug, since there was a merge in June.

However, there remains a related build problem using Lombok v1.18.2 with Gradle v4.9. It works fine with Gradle v4.8.1 (as workaround). The recent fix has not addressed all of the build problem(s) I feel.

  • Lombok v1.18.2

... Exhibits the problem described in: 6120 -- Lombok / Gradle 4.9 problem with Groovy folder names(s) on 30-Jun-2018

  • The build breaks when ever you have a src/main/groovy/ source set for the project using v1.18.2.
  • #6120 was closed as a duplicate for this issue.

I am left to conclude that the 1.18.2 release Does Not Include a fix for this issue. Therefore, a release with this fix is pending?

  • Is that correct?

Or, should I re-report #6120 in the Lombok project as a new (different) issue?

@berngp

This comment has been minimized.

berngp commented Jul 31, 2018

I am running into the same issue that @aplatypus is expressing even after upgrading ti Lombok v1.18.2. The issue presents itself explicitly when running ./gradlew compileGroovyTest. The project doesn't have a src/main/groovy but we do leverage Spock for tests and we have plenty of src/test/groovy files.

@oehme

This comment has been minimized.

oehme commented Jul 31, 2018

Please provide a reproducible example project on Github or as a zip.

@aplatypus

This comment has been minimized.

aplatypus commented Aug 3, 2018

@berngp ... there might be a workaround, or like me, stay on Gradle 4.8. The main reason I need a solution is for up-coming Gradle 5.0.

Anyway, in my case I found that having groovy in my source path was the problem. After using Lombok v1.18.2. If that's the same you could try to keep Groovy files under java/ path.

@oehme ... The reproducible example is in the body of #6120.

In fact any Existing Lombok annotated project you have lying around will produce the problem. Just rename:

mv  src/main/java  src/main/groovy

Build with Gradle 4.8 or 4.8.1 -- works

Fails to build using Gradle 4.9.

@oehme

This comment has been minimized.

oehme commented Aug 3, 2018

I could reproduce the problem, though next time I'd be really grateful if you could just attach a full example project instead of some text snippets. It saves a lot of time on my side, which I can use for fixing :)

The problem affected all forking compilers. The fix will be in Gradle 4.10.

@berngp

This comment has been minimized.

berngp commented Aug 3, 2018

Thanks @aplatypus, will stay with Gradle 4.8 for now. Thank as well for providing the details and the description on how to replicate it.

@oehme I apologize for not providing a full example. I appreciate that you looked into it, were able to replicate it and arrive to a conclusion.

Thanks again @aplatypus & @oehme!

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