Avoid reading System Properties during configuration phase with disableCreatingBuildConfigDuringConfiguration property

[cdsap]

This PR covers the issue described in #52464 and expanded on in discussion #52506.

The Quarkus Gradle plugin extension introduces a new property, disableCreatingBuildConfigDuringConfiguration, which disables EffectConfig initialization during configuration. This avoids reading system properties at configuration time, which can cause configuration cache misses. The EffectConfig still happens during task execution.

To opt out of the EffectConfig / BaseConfig configuration, we still need to provide the required properties used as inputs for GetBuildFiles (QuarkuBuild). To do that, and only when disableCreatingBuildConfigDuringConfiguration is enabled, we provide a Provider that supplies the required system properties.

At the moment, we support two levels of properties:

• System property arguments
• Application properties

For this experimental phase, this is sufficient. If we continue with this approach, we will likely need to replicate the other ConfigSource providers as well.

From the client perspective, the user only needs to enable the flag:

quarkus {
  setDisableCreatingBuildConfigDuringConfiguration(true)
  // ...
}

With this property enabled, a sequence of builds like:

./gradlew build -Da=1
./gradlew build -Da=2

will reuse the configuration cache on the second build.

• Closes: Gradle Configuration cache invalidated by unrelated system property changes #52464

[geoand]

@aloubyansky @radcortez can you please take a look at this?

Thanks

[radcortez]

I'll have a look.

[radcortez]

Being discussed in #52506.

[geoand]

Please rebase this onto main as we don't allow merge commits in PRs.

Thanks

[cdsap]

my mistake @geoand, fixed

[geoand]

🙏

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 55116ed.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[radcortez]

Based on our discussion in #52506 (comment), we recommend providing a new Map of values containing the configurations from PackageConfig and NativeConfig, and using that in the relevant tasks, instead of the full map of configuration values.

You should be able to use io.smallrye.config.ConfigMappings#propertyNamesMatcher, which takes the config classes and then use it to iterate the list of available configurations and match the properties to build a list of the names provided by the mapping classes.

[cdsap]

@radcortez Thanks for the clarifications. I think I have a clear path to follow, but I would need some guidance to cover the next case:

Using the new map and iterating over the different configurations works fine when operating in the task action or outputs. However, there is a case with getBuildInputFiles in the QuarkusBuild task:
https://github.com/quarkusio/quarkus/blob/main/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/QuarkusBuild.java#L160

Because this is an input, iterating over the configurations would resolve the different properties during configuration time. In this PR, I covered the case by iterating "manually" over the configurations for system properties and application properties, but other cases such as environment variables or YAML files are not covered yet.

I’m seeing some issues again with how this input is defined, and I’d appreciate your input here.
cc @aloubyansky

[radcortez]

Because this is an input, iterating over the configurations would resolve the different properties during configuration time. In this PR, I covered the case by iterating "manually" over the configurations for system properties and application properties, but other cases such as environment variables or YAML files are not covered yet.

My proposal was to generate this Map from EffectiveConfig. You can populate the Map from a fully built SmallRyeConfig instance, which already reads all of the sources automatically.

[aloubyansky]

My proposal was to generate this Map from EffectiveConfig. You can populate the Map from a fully built SmallRyeConfig instance, which already reads all of the sources automatically.

@radcortez would that result in reading all the system properties?

[radcortez]

What triggers the read of all system properties is the call to getPropertyNames in:

quarkus/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/EffectiveConfig.java

Lines 119 to 141 in 410c177

	@VisibleForTesting
	static Map<String, String> generateFullConfigMap(SmallRyeConfig config) {
	Set<String> defaultNames = new HashSet<>();
	defaultNames.addAll(configClass(PackageConfig.class).getProperties().keySet());
	defaultNames.addAll(configClass(NativeConfig.class).getProperties().keySet());
	return Expressions.withoutExpansion(new Supplier<Map<String, String>>() {
	@Override
	public Map<String, String> get() {
	Map<String, String> properties = new HashMap<>();
	for (String propertyName : config.getPropertyNames()) {
	ConfigValue configValue = config.getConfigValue(propertyName);
	// Remove defaults coming from PackageConfig and NativeConfig, as this Map as passed as
	// system properties to Gradle workers and, we loose the ability to determine if it was set by
	// the user to evaluate deprecated configuration
	if (configValue.getValue() != null && (!defaultNames.contains(configValue.getName())
	|| !configValue.isDefault())) {
	properties.put(propertyName, configValue.getValue());
	}
	}
	return unmodifiableMap(properties);
	}
	});
	}

The problem is that the Map generated by that method is passed down to BaseConfig and used to determine the caching properties:

quarkus/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/BaseConfig.java

Lines 29 to 68 in 408dee6

	BaseConfig(EffectiveConfig config) {
	manifest = new Manifest();
	packageConfig = config.getConfig().getConfigMapping(PackageConfig.class);
	nativeConfig = config.getConfig().getConfigMapping(NativeConfig.class);
	// populate the Gradle Manifest object
	PackageConfig.JarConfig.ManifestConfig manifestConfig = packageConfig.jar().manifest();
	manifest.attributes(manifestConfig.attributes());
	manifestConfig.sections().forEach((section, attribs) -> manifest.attributes(attribs, section));
	values = config.getValues();
	}
	PackageConfig packageConfig() {
	return packageConfig;
	}
	NativeConfig nativeConfig() {
	return nativeConfig;
	}
	PackageConfig.JarConfig.JarType jarType() {
	return packageConfig().jar().type();
	}
	Manifest manifest() {
	return manifest;
	}
	Map<String, String> cachingRelevantProperties(List<String> propertyPatterns) {
	List<Pattern> patterns = propertyPatterns.stream().map(s -> "^(" + s + ")$").map(Pattern::compile)
	.collect(Collectors.toList());
	readMissingEnvVariables(propertyPatterns);
	Predicate<Map.Entry<String, ?>> keyPredicate = e -> patterns.stream().anyMatch(p -> p.matcher(e.getKey()).matches());
	return values.entrySet().stream()
	.filter(keyPredicate)
	.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (s, s2) -> {
	throw new IllegalArgumentException("Duplicate key");
	}, TreeMap::new));
	}

Hence my suggestion to generate only a Map with the PackageConfig and NativeConfig properties to pass down to that caching method.