Resolve Gradle Configuration Caching Issues When Executing AsciidoctorTask #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tylerbertrand
changed the title
… to replace references to asciidoctorj.options and asciidoctorj.attributes
…e an input annotation
…to within the jrubyLessDependenciesProvider
tylerbertrand
force-pushed
the
tb/configuration-caching
branch
from
2d63794 to
bc07f63
Compare
Shorten too-long lines Replace opt.isPresent() with opt.present Remove unnecessary getters Replace call to getAsciidoctorJExtension() with asciidoctorJExtension Remove unnecessary package reference Correct instance ordering
tylerbertrand
force-pushed
the
tb/configuration-caching
branch
from
a756f4a to
47262ef
Compare
cdsap
reviewed
Jun 20, 2024
| @@ -750,26 +807,19 @@ class AbstractAsciidoctorTask extends AbstractJvmModelExecTask<AsciidoctorJvmExe | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Try to do this without a detached configuration | |||
| private FileCollection jrubyLessConfiguration(List<Dependency> deps) { | |||
There was a problem hiding this comment.
just FYI, in a recent conversation with the BT team, they mentioned:
detached configurations are probably not what you want. "detached" means that almost everything that
comes from project state is separate. We were going to deprecate detached configurations from extending
project configurations,
cdsap
approved these changes
Jun 20, 2024
runningcode
reviewed
Jul 9, 2024
| @@ -719,19 +776,19 @@ class AbstractAsciidoctorTask extends AbstractJvmModelExecTask<AsciidoctorJvmExe | |||
| if (deps.empty && closurePaths.empty) { | |||
| projectOperations.fsOperations.emptyFileCollection() | |||
| } else if (closurePaths.empty) { | |||
| jrubyLessConfiguration(deps) | |||
| jrubyLessDependenciesProvider.get() | |||
There was a problem hiding this comment.
deps doesn't need to be passed in here as it has been moved to within the provider
| private final Property<FileCollection> jvmClasspath | ||
| private final List<Provider<File>> gemJarProviders = [] | ||
|
|
||
| protected final MapProperty<String, Object> optionsProperty = project.objects.mapProperty(String, Object) |
There was a problem hiding this comment.
why are these protected instead of private?
There was a problem hiding this comment.
I had run into some unknown property exception issues that I thought were resolved by this, but after testing now, it works just fine with them private, so I've made that change.
| this.asciidoctorj = extensions.create(AsciidoctorJExtension.NAME, AsciidoctorJExtension, this) | ||
|
|
||
| def asciidoctorj = extensions.create(AsciidoctorJExtension.NAME, AsciidoctorJExtension, this) | ||
| this.optionsProperty.set(asciidoctorj.options) |
There was a problem hiding this comment.
are we setting default values here? what does this block do?
There was a problem hiding this comment.
Yep, setting default values
…actAsciidoctorTask to be private There is no reason for them to be protected
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the Gradle configuration caching issues originating from this project when executing
org.asciidoctor.gradle.jvm.AsciidoctorTask. There are some remaining issues, but those are caused by the grolifant library.The configuration caching issues in this project were primarily tasks referencing types incompatible with configuration caching. The references to these types were mostly held by
AsciidoctorJExtension, whichAbstractAsciidoctorTaskheld a reference to. To resolve this, the individual properties ofAsciidoctorJExtensionwere moved directly toAbstractAsciidoctorTaskin a configuration cache-compatible way. No input annotations were added to these properties to maintain parity with the current task behavior. The property values are wired from the extension to the task properties inAbstractAsciidoctorTask's constructor. In such cases where providers are used and referencingAsciidoctorJExtensionis required, an@Internalgetter is used to access theAsciidoctorJExtensioninstance. This@Internalgetter prevents Gradle from attempting to serializeAsciidoctorJExtensionto the configuration cache.One behavioral change is that
AbstractAsciidoctorTaskno longer has the ability to write back toAsciidoctorJExtensionsince the reference from the task to the extension was removed. The methods that allowed this were updated to write to the properties that now reside in the task. As it stands I'm not sure what the full impact of that change is. Additionally, since this reference was removed, some methods were copied fromAsciidoctorJExtensiontoAbstractAsciidoctorTaskwhere needed. This should probably be done in a better way than just copying the methods, but I would like to get input first since this changes the relationship between the extension and the task.The remaining configuration caching issues are shown in the following report
