Skip to content

Deprecating Extension.optional #11201

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.

Sign up for GitHub

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

Merged
timja merged 1 commit into from
Oct 16, 2025
Merged

Deprecating Extension.optional #11201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/src/main/java/hudson/Extension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
@Retention(RUNTIME)
@Target({TYPE, FIELD, METHOD})
@Documented
public @interface Extension {

Check warning on line 75 in core/src/main/java/hudson/Extension.java

View check run for this annotation

ci.jenkins.io / Java Compiler

checkstyle:check 

NORMAL:
should be marked @retention(RetentionPolicy.SOURCE)
/**
* Used for sorting extensions.
*
Expand All @@ -87,7 +87,10 @@
/**
* If an extension is optional, don't log any class loading errors when reading it.
* @since 1.358
* @deprecated This is very difficult to use correctly and rarely what you actually wanted.
* Use {@code OptionalExtension} from the {@code variant} plugin instead.
*/
@Deprecated
boolean optional() default false;

/**
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/hudson/ExtensionFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ public DefaultGuiceExtensionAnnotation() {
super(Extension.class);
}

@SuppressWarnings("deprecation")
@Override
protected boolean isOptional(Extension annotation) {
return annotation.optional();
Expand Down Expand Up @@ -779,6 +780,7 @@ public void scout(Class extensionType, Hudson hudson) {
}
}

@SuppressWarnings("deprecation")
private Level logLevel(IndexItem<Extension, Object> item) {
return item.annotation().optional() ? Level.FINE : Level.WARNING;
}
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/java/hudson/lifecycle/ExitLifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
package hudson.lifecycle;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.Extension;
import hudson.util.BootFailure;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -44,7 +43,6 @@
* @author Alon Bar-Lev
*/
@Restricted(NoExternalUse.class)
@Extension
public class ExitLifecycle extends Lifecycle {

private static final Logger LOGGER = Logger.getLogger(ExitLifecycle.class.getName());
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/hudson/lifecycle/Lifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.ExtensionPoint;
import hudson.Functions;
import hudson.PluginManager;
import hudson.Util;
Expand Down Expand Up @@ -62,7 +61,7 @@
* @author Kohsuke Kawaguchi
* @since 1.254
*/
public abstract class Lifecycle implements ExtensionPoint {
public abstract class Lifecycle {
Copy link
Member Author

@jglick jglick Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Lifecycle does not even use the extension loader to begin with! Traditionally ExtensionPoint was just used as a very hand-wavy indicator for “things you might want to pay attention to in Javadoc as a plugin author”, rather than specifically meaning “a type which can be subtyped and marked with @Extension to register”. (A broad category of such mistakes is placing ExtensionPoint on a Describable. It is the Descriptor which is the extension point.)

Copy link
Member

@daniel-beck daniel-beck Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traditionally ExtensionPoint was just used as a very hand-wavy indicator for “things you might want to pay attention to in Javadoc as a plugin author”, rather than specifically meaning “a type which can be subtyped and marked with @Extension to register”.

Isn't that an indicator this should stay?

See also class Javadoc for PluginServletFilter in which this quirk is made explicit.

Copy link
Member Author

@jglick jglick Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jenkins/core/src/main/java/hudson/util/PluginServletFilter.java

Lines 57 to 58 in c9eaa5d

* While this class by itself is not an extension point, I'm marking this class
* as an extension point so that this class will be more discoverable.
predates #7892 which is a true extension point.

Copy link
Member

@daniel-beck daniel-beck Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point stands, it's not just tradition, but also documented as such.

Copy link
Member Author

@jglick jglick Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can correct the documentation: #11210

Copy link
Member

@daniel-beck daniel-beck Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the idea is that ExtensionPoint is only for things intended to be @Extension'ed, then

jenkins/core/src/main/java/hudson/ExtensionPoint.java

Lines 35 to 41 in c97fecc

* Marker interface that designates extensible components
* in Jenkins that can be implemented by plugins.
*
* <p>
* See respective interfaces/classes for more about how to register custom
* implementations to Jenkins. See {@link Extension} for how to have
* Jenkins auto-discover your implementations.
should also be rephrased accordingly. At the moment it's ambiguous (at least to me), d4c6a4b kept the original documentation referring to the specific classes explaining how to register (which would apply to the case of PluginServletFilter), only adding a mention of @Extension.

private static Lifecycle INSTANCE = null;

/**
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/java/hudson/lifecycle/SystemdLifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import com.sun.jna.Native;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -17,7 +16,6 @@
* @author Basil Crow
*/
@Restricted(NoExternalUse.class)
@Extension(optional = true)
Copy link
Member Author

@jglick jglick Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear why this was “optional”. Of course this would only make sense on servers with systemd. But it would get loaded as an extension in all cases.

public class SystemdLifecycle extends ExitLifecycle {

private static final Logger LOGGER = Logger.getLogger(SystemdLifecycle.class.getName());
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/jenkins/diagnosis/HsErrPidList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@
*
* @author Kohsuke Kawaguchi
*/
@Extension(optional = true) @Symbol("hsErrPid")
// TODO why would an extension using a built-in extension point need to be marked optional?
Copy link
Member Author

@jglick jglick Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who knows.

@Extension @Symbol("hsErrPid")
public class HsErrPidList extends AdministrativeMonitor {
/**
* hs_err_pid files that we think belong to us.
Expand Down
Loading