Completely disable all relevant build steps of extensions (except FeatureBuildItem production) when their "enabled" configuration switch is set to false

[yrodiere]

In particular, when such an extension is disabled:

• Don't display the feature in logs on startup => changed following comments, see conversation below.
• Don't enable SSL
• Don't add reflective classes in native mode

EDIT: Also, regardless of whether the extension is enabled or not, always display the feature in logs on startup.

Creating this PR as draft, because it's based on #26965 , which must be merged first. => Done

I separated this PR from #26965 because #26965 doesn't alter the behavior of any extension, while this PR does (but for the better, IMO).

[quarkus-bot]

/cc @brunobat, @ebullient, @jmartisk, @radcortez

[gsmet]

Don't display the feature in logs on startup

Can't say I'm excited about this particular one. For me, it's important to know that the feature has actually be detected by Quarkus even if not active.

[yrodiere]

Can't say I'm excited about this particular one. For me, it's important to know that the feature has actually be detected by Quarkus even if not active.

We'll have to make up our minds then, because other extensions (Micrometer, ...) hide the feature when they are disabled:

quarkus/extensions/micrometer/deployment/src/main/java/io/quarkus/micrometer/deployment/MicrometerProcessor.java

Lines 86 to 89 in 8816386

	@BuildStep(onlyIf = MicrometerEnabled.class)
	FeatureBuildItem feature() {
	return new FeatureBuildItem(Feature.MICROMETER);
	}

For context, here's the log message we're talking about:

2022-07-27 15:31:25,238 INFO [io.quarkus] (Quarkus Main Thread) Installed features: [agroal, cdi, hibernate-orm, hibernate-orm-panache, hibernate-search-elasticsearch, jdbc-postgresql, narayana-jta, resteasy-reactive, resteasy-reactive-jackson, smallrye-context-propagation, vertx]

We can argue whether it's clear that "installed" doesn't mean "enabled". Personally I find the distinction too confusing and I would hide the extensions when they are disabled, but in the end I don't really care either way, as long as we're consistent across Quarkus.

FWIW, we also use FeatureBuildItem to forbid having two (enabled?) extensions with the same name:

quarkus/core/deployment/src/main/java/io/quarkus/deployment/steps/MainClassBuildStep.java

Lines 257 to 264 in c09cf50

	List<String> featureNames = new ArrayList<>();
	for (FeatureBuildItem feature : features) {
	if (featureNames.contains(feature.getName())) {
	throw new IllegalStateException(
	"Multiple extensions registered a feature of the same name: " + feature.getName());
	}
	featureNames.add(feature.getName());
	}

Not sure what to think about that. We probably wouldn't want to have two extensions providing the same feature even if one is disabled?

EDIT: Though that could be useful for users who end up having two (incompatible) extensions in the classpath that provide the same feature... Two alternative implementations with a common API maybe, but integrating with different technologies/environments? I don't know...

[gsmet]

Well I don't know about Micrometer but the purpose of this message has always been to inform about the features detected by Quarkus.
Enabled or not, that's a matter of config.

We can decide to change that. But... my personal opinion is that it's not a good idea. Knowing that an extension has actually been detected is an important piece of information and why we have this line in the log.

FWIW, we also use FeatureBuildItem to forbid having two (enabled?) extensions with the same name

I think this is probably something that predates capabilities. But it's still a good thing in my opinion as having 2 features with the same name would be confusing.

[yrodiere]

@sberyozkin could you please confirm this isn't a problem for the various oidc extensions?

Hey @sberyozkin, bump :) Any opinion on this change for the oidc extensions? If you don't particularly care either way, I think we'll go ahead, since other extension owners agree with the change.

[pedroigor]

@yrodiere I'm really happy to see this change happening. It would help a lot Keycloak and open the doors to lot of improvements. For instance, quarkiverse/quarkus-vault#12 (comment). AFAIK, this PR should help with that.

As per the security extensions, I would leave it to @sberyozkin. But I don't see an issue for keycloak-authorization.

[pedroigor]

@yrodiere I guess the changes here are not yet allowing disabling build steps conditionally, right? For instance, Keycloak is basically a Quarkus extension and we would like to conditionally disable build steps depending on the user configuration when running re-augmentation.

[yrodiere]

@yrodiere I'm really happy to see this change happening. It would help a lot Keycloak and open the doors to lot of improvements. For instance, quarkiverse/quarkus-vault#12 (comment). AFAIK, this PR should help with that.

I'm not sure it will, since this PR only disables a few build steps when the (existing) *.enabled configuration properties are set to false. The @BuildSteps annotation has already been merged in a previous PR, so you can probably already experiment with it. It's mostly just syntactic sugar, though; see #26965 / #26980 .

As to disabling extensions at runtime, which I think was mentioned in the issue you linked, that's still not possible, unfortunately. But maybe @BuildSteps alone is enough?

[yrodiere]

@yrodiere I guess the changes here are not yet allowing disabling build steps conditionally, right?

This PR doesn't change anything regarding that, no.

For instance, Keycloak is basically a Quarkus extension and we would like to conditionally disable build steps depending on the user configuration when running re-augmentation.

That was already possible, I think? Just use @BuildStep(onlyIf = MyCondition.class) on that step and have the relevant configuration injected in MyCondition. See https://quarkus.io/guides/writing-extensions#conditional-step-inclusion

Recently I added @BuildSteps (with a trailing s) that can be applied at the type level, which is more convenient, but is just syntactic sugar.

[pedroigor]

Recently I added @BuildSteps (with a trailing s) that can be applied at the type level, which is more convenient, but is just syntactic sugar.

Yeah, using a condition to a BuildStep works and we are using that. But I did not know about the top-level class BuildSteps.

Can I expect that when a class is using BuildSteps the ExtensionLoader won't even try to resolve methods from the build class using reflection?

[pedroigor]

Btw, also related to what we are looking for #22122.

[yrodiere]

Can I expect that when an extension is using BuildSteps the ExtensionLoader won't even try to resolve methods from the build class using reflection?

No, reflection will still happen. Conditions are evaluated after ExtensionLoader does its job (which is, in particular, determining what the condition classes are, but does not include evaluating the conditions).

I don't know if your suggestion can be implemented either: I'm not too familiar with this code, and there's so many levels of callbacks in this code that it's hard to say what's going on for sure :) But someone obviously made the choice to delay evaluation of conditions, and I'm guessing there's a reason for that.

Maybe it would be better to open a dedicated issue to discuss that.

[pedroigor]

@yrodiere Sure, thanks for your input. This change is still valuable and we should be looking at how adapt to it.

[gsmet]

@yrodiere I think we should pursue with this before we end up with conflicts.

[yrodiere]

@sberyozkin could you please confirm this isn't a problem for the various oidc extensions?

Hey @sberyozkin, bump :) Any opinion on this change for the oidc extensions? If you don't particularly care either way, I think we'll go ahead, since other extension owners agree with the change.

No answer, so I'll assume it's good to go.

Thanks everyone! Merging

[yrodiere]

... I'll let CI run first though :-)