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

Fix DelombokTask and InstallLombokTask under Gradle 6.4 #77

Merged
merged 1 commit into from
May 10, 2020
Merged

Fix DelombokTask and InstallLombokTask under Gradle 6.4 #77

merged 1 commit into from
May 10, 2020

Conversation

chadlwilson
Copy link
Contributor

@chadlwilson chadlwilson commented May 7, 2020

We are not allowed to call setMain during exec phase on JavaExec because it is now final due to being a Property<String> for lazy configuration in 6.4. See gradle/gradle#12841

The tests assertions as left implemented here continue to work up until 6.3 but assertions would need to change for 6.4 as the value is no longer set on the mock; and has its own property.

Tested this approach works at runtime on 6.3 and 6.4 in any case - fixes #76

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #77 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #77      +/-   ##
============================================
- Coverage     98.52%   98.48%   -0.05%     
- Complexity       21       23       +2     
============================================
  Files             5        5              
  Lines            68       66       -2     
  Branches          7        7              
============================================
- Hits             67       65       -2     
  Partials          1        1              
Impacted Files Coverage Δ Complexity Δ
...franzbecker/gradle/lombok/task/DelombokTask.groovy 100.00% <100.00%> (ø) 3.00 <2.00> (+1.00)
...becker/gradle/lombok/task/InstallLombokTask.groovy 100.00% <100.00%> (ø) 2.00 <1.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27f2811...a6800b7. Read the comment docs.

Copy link
Owner

@franzbecker franzbecker left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 👍

@franzbecker franzbecker merged commit 9596c62 into franzbecker:master May 10, 2020
@franzbecker
Copy link
Owner

Released it as 4.0.0

@chadlwilson chadlwilson deleted the gradle-64 branch May 10, 2020 13:21
@chadlwilson
Copy link
Contributor Author

Thanks @franzbecker - working fine for us :-)

@stevesaliman
Copy link

I ran into this same issue while looking into Liquibase Gradle Plugin Issue 71. What we found is that calling setMain() in the config method avoids the runtime error, it doesn't actually work. In our case, we were setting a new value for main in an extension, but config was being called before the extension was processed, so we only ever got the default value for main.

If you add main 'anything.at.all' to the lombok block of your build.gradle, does it have any effect?

@chadlwilson
Copy link
Contributor Author

Hi @stevesaliman - I believe this works for the lombok plugin because there isn't an obvious use case for overriding the main class given this is just a convenience extension task that calls Lombok's main with the project classpath.

I understand that it wouldn't work if one attempted to override it in the build.gradle like you suggest though.

My understanding from this gradle issue is that you have to use the convention approach if you want to support overriding; which is what it looks like you have gone ahead with in the Liquibase plugin via the internal conventionMapping API.

I deliberately didn't want to go that direction as I couldn't quickly figure out what backwards compatibility that would give us with Gradle versions given it was an internal API.

@stevesaliman
Copy link

I'm actually more concerned with forward compatibility using an internal API :-) I have a feeling our solution will not age well, but we have a more likely use case for needing to override the main class for now.

I just wanted to give you the heads up in case you hadn't run across that issue yet.

@chadlwilson
Copy link
Contributor Author

Thanks - much appreciated!

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.

Extensions of gradle-lombok tasks fail with Gradle 6.4
3 participants