-
Notifications
You must be signed in to change notification settings - Fork 6
Make gradle-revapi
configuration cache friendly
#13
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
base: main
Are you sure you want to change the base?
Conversation
gradle-revapi
configuration cache friendly
getProject().getObjects().property(Justification.class); | ||
|
||
public RevapiAcceptBreakTask() { | ||
getOutputs().upToDateWhen(_ignored -> false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to doNotTrackState
in RevapiPlugin.java
@@ -92,8 +79,4 @@ private void ensurePresent(Property<?> prop, String option) { | |||
throw new IllegalArgumentException("Please supply the --" + option + " param to this task"); | |||
} | |||
} | |||
|
|||
private GroupNameVersion oldGroupNameVersion() { | |||
return getProject().getExtensions().getByType(RevapiExtension.class).oldGroupNameVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the use of getProject
that was breaking configuration cache the rest of the changes are just about converting to an abstract class - can just remove this if we want a minimum PR
revapiIgnores(), | ||
ConjureProjectFilters.forProject(getProject()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the use of getProject
that was breaking configuration cache the rest of the changes are just about converting to an abstract class - can just remove this if we want a minimum PR
private final Property<String> oldGroup; | ||
private final Property<String> oldName; | ||
private final ListProperty<String> oldVersions; | ||
private final Provider<GroupAndName> oldGroupAndName; | ||
|
||
@Nested | ||
protected abstract GitVersionUtils getGitVersionUtils(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made GitVersionUtils
injectable as I think it is a bit cleaner but we could instead keep previousGitTags
static and just pass in the dir and provider factory if we think that is tidier
getProject() | ||
.getTasks() | ||
.withType(RevapiAcceptBreakTask.class) | ||
.getByName(RevapiPlugin.ACCEPT_BREAK_TASK_NAME) | ||
.getPath()); | ||
templateData.put("acceptBreakTask", getAcceptAllBreaksTaskPath().get()); | ||
templateData.put( | ||
"acceptAllBreaksProjectTask", | ||
getProject() | ||
.getTasks() | ||
.withType(RevapiAcceptAllBreaksTask.class) | ||
.getByName(RevapiPlugin.ACCEPT_ALL_BREAKS_TASK_NAME) | ||
.getPath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the bad use of getProject
similarly the other changes are all about making it abstract can just do this bit if we prefer
} | ||
|
||
private GroupNameVersion oldGroupNameVersion() { | ||
return getProject().getExtensions().getByType(RevapiExtension.class).oldGroupNameVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as previous this is the actual call to getProject
that break configuration cache other changes are about making abstract
import spock.util.environment.RestoreSystemProperties | ||
|
||
class RevapiSpec extends IntegrationSpec { | ||
class RevapiSpec extends ConfigurationCacheSpec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigurationCacheSpec
is built on IntegrationTestKitSpec
so runTask
now returns a BuildResult
most of the changes here are focused on converting to using BuildResult
private Git git | ||
|
||
def setup() { | ||
git = new Git(projectDir) | ||
System.setProperty("ignoreDeprecations", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to build.gradle
executionResult.wasSkipped(':revapi') | ||
def buildResult = runTasksSuccessfully('revapi') | ||
assert buildResult.task(':revapiAnalyze').outcome == TaskOutcome.SKIPPED | ||
assert buildResult.task(':revapi').outcome == TaskOutcome.SKIPPED | ||
} | ||
|
||
def 'when the previous git tag has failed to publish, it will look back up to a further git tag'() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails on main
not sure if it should be @Ignore
?
Before this PR
gradle-revapi
had a range of issues making it not configuration cache friendly:project.exec
to runGit
commandsTask.project
withinConjureProjectFilters
,RevapiAcceptBreakTask
,RevapiAnalyzeTask
,RevapiReportTask
andRevapiVersionOverrideTask
After this PR
Converted
GitVersionUtils
to a manage type so it can be injected via@Nested
and switched to the configuration cache friendlyProviderFactory.exec
Converted
ConjureProjectFilters
,RevapiAcceptBreakTask
,RevapiAnalyzeTask
,RevapiReportTask
andRevapiVersionOverrideTask
to abstract classes and removed theTask.project
calls by passing in the needed fields as inputsAdded the
gradle-plugin-testing
plugin and usedConfigurationCacheSpec
to allowRevapiSpec
to test configuration cache issues==COMMIT_MSG==
Make
gradle-revapi
configuration cache friendly==COMMIT_MSG==
Possible downsides?