[Configuration cache] Dependent OsDetectorPlugin is not compliant with configuration cache

[voidzcy]

As mentioned in #406 (comment), the dependent OsDetectorPlugin uses JDK APIs to read system properties, which violates the requirement for configuration cache:

Plugins and build scripts should not read system properties directly using the Java APIs at configuration time. Instead, these system properties must be declared as a potential build input by using the value supplier APIs.

That needs to be fixed either in osdetector-gradle-plugin or having some workaround in protobuf-gradle-plugin if possible.

Users of protobuf-gradle-plugin are still able to take advantage of configuration cache feature with --configuration-cache=warn argument (ignoring errors of InvalidUserCodeException, which are considered to be harmless, our existing test does so).

[voidzcy]

There is no easy way to mitigate this now.

• The osdetector-gradle-plugin delegates reading system properties to the os-maven-plugin, which calls JDK APIs.
• Reading system properties happens in configuration phase (the afterEvaluate hook), we are not able to wrap system property reading into a build service that is used in task execution.

We probably would need to migrate away from os-mavan-plugin.

[gavra0]

@voidzcy Can we implement this in the Protobuf plugin? We probably need to support just a couple of basic os/arch combinations.

[voidzcy]

We need to read the system property in configuration time to determine the dependency notation in a map form for protoc. I am not sure if a map with lazily evaluated values would work.

Even with that, we would need to implement the system properties/files reading with Gradle's Provider approach by ourselves just in order to get the classifier to be used the dependency. Are you able to contribute?

[ejona86]

I don't see any need to reimplement this logic. It seems very easy to enhance os-detector-maven to work with our needs. It seems Detector is pretty close already. It already requires overriding some protected methods for logging; we would just add a few for accessing System. It looks like there is only one static method using System, and that can just be overloaded.

[voidzcy]

We can try wrapping project.osdetector.classifier into a Gradle Provider and chain it with Provider.forUseAtConfigurationTime(). A normal Provider would not work as it will just trigger the read operation to happen. But still my biggest concern is if the dependency notation can contain some lazily evaluated value and it cannot be resolved until the execution phase.

We are not able to make os-maven-plugin work as this requires Gradle API.

[voidzcy]

I tried using MapProperty, but no luck. Would need more involved changes.

[voidzcy]

I've experimented locally by replacing

• System.getProperty() with ProviderFactory.systemProperty(name).forUseAtConfigurationTime().getOrNull()

• File reading with

RegularFile file = getProjectLayout().getProjectDirectory().file(fileName);
ProviderFactory.fileContents(file).getAsBytes().forUseAtConfigurationTime().getOrNull();

It works with configuration cache.

Note this approach still reads system properties/files at configuration phase (in a configuration cache compliant way), which means changing to their values will cause cache entry invalidation and reconfiguration. But since the os-detector plugin is mostly reading OS properties/files, they are not expect to change between builds in normal cases. So configuration cache would still be beneficial for most users.

I've created trustin/os-maven-plugin#47 for os-maven-plugin. Once that gets accepted, I will make changes to osdetetor-gradle-plugin and then solve this issue here.

[voidzcy]

osdetector-gradle-plugin 1.7.0 is now compliant with configuration caching. Tests for this plugin have been fully enabled in #467.