ConsoleAppender: Fix missing default Target value

[rschmitt]

This adds a trivial missing code path to implement the documented default value for Target.

[github-actions]

Job	Requested goals	Build Tool Version	Build Outcome	Build Scan®
build-macos-latest	clean install	3.9.8	✅
build-ubuntu-latest	clean install	3.9.8	✅
build-windows-latest	clean install	3.9.8	✅

Generated by gradle/develocity-actions

[ppkarwasz]

Hi @rschmitt,

Since the method you improved has been deprecated since version 2.7, I don't think it needs fixing.
On the other hand it might be useful adding an Objects.requireNonNull check on the builder's setTarget method to throw an early exception if the value is invalid.

[rschmitt]

@ppkarwasz It absolutely needs fixing, this is a regression and it's breaking code that I'm trying to upgrade from 2.24.3. I don't know exactly what the relevant call path is or what changed in it, all I know is that I have a bunch of broken plugin code whose tests are failing because createAppender has started silently returning null.

[vy]

@ppkarwasz, while I do agree with you, the fix @rschmitt is proposing to the deprecated createAppender is already implemented for another deprecated createAppender just a few lines above. I'm inclined to accept @rschmitt's suggestion. WDYT?

@rschmitt, how feasible is it for you to fix code paths accessing deprecated Log4j APIs?

[rschmitt]

how feasible is it for you to fix code paths accessing deprecated Log4j APIs?

@vy I already have a variety of issues associated with 2.25 that I am working on fixing. For example, the JSpecify annotations added in commits like a1dfa85 are breaking JDK8 builds that are using Mockito to mock APIs with those annotations, because they don't have those annotations on their test classpath. (This appears to be a bug in core reflection. It works fine on JDK17.) Other builds are failing due to the combination of -Werror and -Xlint:classfile, due to those same annotations: if they're not on the compilation classpath, a linter warning gets emitted due to -Xlint:classfile, which gets upgraded to a compilation error by -Werror. One library was using ThrowableProxy, and its tests started failing because setThrownProxy was changed to a no-op; I had to patch it to use Throwable/setThrown.

I haven't submitted issues or bug reports for any of these things, because they're not obviously regressions in Log4j itself, and I have to either fix the packages that can't upgrade or patch Log4j itself to work around the issue. This issue OTOH is clearly a regression, because the method doesn't do what the Javadoc says it should, and there's some sort of validation logic in 2.25 that is surfacing the problem. Since this is a bug, I'd rather fix it upstream than change all the consumers to use non-deprecated code paths, since that would just take time away from the issues I've listed in the previous paragraph, which I'm still working on.

[ppkarwasz]

@ppkarwasz, while I do agree with you, the fix @rschmitt is proposing to the deprecated createAppender is already implemented for another deprecated createAppender just a few lines above. I'm inclined to accept @rschmitt's suggestion. WDYT?

I am OK with the proposed patch, I was just wondering if we should also protect users that use newBuilder().setTarget(null) or we should throw a fail-fast NPE. I would rather prefer the second one.

[garydgregory]

I think the exception should happen on construction to allow for setters to be called over and over.
In general, this design allows for co-dependent values to change but only be validated in the constructor. Otherwise, each setter in the builder needs to know the invariants for all the other values in the builder it depends on. This could be solved with a builder validation method that takes into account the state of the whole builder but it seems to me to be cleaner to have the constructor do the validation, since that's what really needs to be valid. If the constructor is public or protected, then it must do the validation anyway. And I wouldn't want to have validation code in two places.

[ppkarwasz]

Hi @rschmitt,

Sorry to hear you've run into so many issues with the upgrade. Fortunately, we encountered all of these problems before and we’re happy to help.

I already have a variety of issues associated with 2.25 that I am working on fixing. For example, the JSpecify annotations added in commits like a1dfa85 are breaking JDK8 builds that are using Mockito to mock APIs with those annotations, because they don't have those annotations on their test classpath. (This appears to be a bug in core reflection. It works fine on JDK17.)

This is a known issue caused by JDK-8152174. We also test on JDK 8 in our CI pipeline, and have documented the workaround here:

logging-log4j2/log4j-parent/pom.xml

Lines 1160 to 1171 in 286191e

	<!-- There are certain Java 8 bugs[1] that cause Mockito failures[2].
	Adding necessary dependencies to workaround them.
	[1] https://bugs.openjdk.org/browse/JDK-8152174
	[2] https://github.com/mockito/mockito/issues/1449 -->
	<dependencies>
	<dependency>
	<groupId>org.jspecify</groupId>
	<artifactId>jspecify</artifactId>
	<version>${jspecify.version}</version>
	<scope>test</scope>
	</dependency>
	</dependencies>

Other builds are failing due to the combination of -Werror and -Xlint:classfile, due to those same annotations: if they're not on the compilation classpath, a linter warning gets emitted due to -Xlint:classfile, which gets upgraded to a compilation error by -Werror.

Yes, we’ve run into this too. There’s a detailed discussion on the dev@logging mailing list here:
https://lists.apache.org/thread/7tkztw94v1rfn1l2k7f42nzx3jqpsrff

A few key takeaways:

• Oracle does not consider -Xlint:classfile warnings problematic, even when emitted for runtime-invisible annotations.
• For Gradle users, we publish metadata that includes spotbugs-annotations and similar only as compile-time dependencies, which suppresses the warning.
• Our initial attempt to address this in Gradle metadata unfortunately broke compatibility for users compiling with --release 8, so version 2.25.0 is incompatible with that setup.

One library was using ThrowableProxy, and its tests started failing because setThrownProxy was changed to a no-op; I had to patch it to use Throwable/setThrown.

Yes, that should be documented in the release notes. Did we do it properly?

I haven't submitted issues or bug reports for any of these things, because they're not obviously regressions in Log4j itself, and I have to either fix the packages that can't upgrade or patch Log4j itself to work around the issue.

Thank you for not opening new issues. We had similar issue reports before, but we currently don’t have a clean long-term solution (short of bytecode manipulation to strip annotations). Suggestions are always welcome.

[rschmitt]

I'm actually releasing Log4j 2.25.1 right now. I've fixed all the known issues pretty much along the lines you've described, such as adding test dependencies on jspecify and disabling -Werror. This PR is the only one I'd like to see merged; all the other breakage is typical Hyrum's Law stuff.

We had similar issue reports before, but we currently don’t have a clean long-term solution (short of bytecode manipulation to strip annotations). Suggestions are always welcome.

I suppose the long-term solution is to wait for the Valhalla technology, in this case null-restricted and nullable types. Until then, nullability can only be modeled using third-party annotations, and most dependency managers don't have a concept of transitive compile-only dependencies (what Gradle calls compileOnlyApi). Even the Gradle solution seems weird to me, because a lot of these other annotation libraries certainly appear to be implementation details of Log4j's build process.

Personally, I don't believe in linters. I think they provide very marginal benefits at the cost of significantly more fragile and complex builds. This whole thing with the jspecify annotations is just a story of everyone's linters fighting each other and breaking perfectly good builds over nothing.

[vy]

@rschmitt, would you mind adding a changelog entry, please?

[rschmitt]

@vy Will do, I just didn't realize that this change merited a changelog entry.

[ppkarwasz]

I’m wondering about the best approach to maintain synchronization between 2.x (stable) and main (next major release).

@vy, what do you think about consistently performing one of these actions before merging any PR to 2.x:

• Create a corresponding PR to cherry-pick changes into main.
• Open an issue explicitly tracking the synchronization of each PR merged into 2.x.
• Clearly document in the PR comment why a change does not need to be ported to main.

This approach would ensure we avoid accumulating backlog and helps us verify merges effectively.

In this specific case, the change does not need porting because the modified method was already removed from main (verified).

[jvz]

That approach sounds good to me. It might be useful to add some labels to indicate whether a PR has been verified to be applied to all applicable branches (or the inverse: a label to indicate that a PR still needs to be verified as ported).

[vy]

consistently performing one of these actions before merging any PR to 2.x

I understand the motivation for keeping main in sync. with 2.x. Given main will soon be released as Log4j 3 since... 2018-01-30 (041fe8f) and we can hardly afford time for 2.x maintenance, I am afraid of jeopardizing the Log4j 2 development by introducing a process that blocks 2.x fixes for the sake of main.

1. @jvz, @ppkarwasz, do you have capacity to make significant commitment to Log4j 3 development once it is out?
2. If yes, can we introduce a process that does not block 2.x development?

[ppkarwasz]

I am afraid of jeopardizing the Log4j 2 development by introducing a process that blocks 2.x fixes for the sake of main.

What I am proposing here should not block development: it takes 5 minutes to open a new issue or mark that a PR is not applicable to main.

I would love if you could cherry-pick the changes, but I am not doing it myself, so I would leave it as an option.

[garydgregory]

I am afraid of jeopardizing the Log4j 2 development by introducing a process that blocks 2.x fixes for the sake of main.

What I am proposing here should not block development: it takes 5 minutes to open a new issue or mark that a PR is not applicable to main.

I would love if you could cherry-pick the changes, but I am not doing it myself, so I would leave it as an option.

Yes, please don't hold up fixing and/or releasing g from 2.x 👍

[jvz]

I'm not looking for a process blocker. I primarily want to keep track of todo items so they're not lost. The plan is to be more involved with 3.x maintenance, yes.

[rschmitt]

Just out of curiosity, is Log4j 3 targeting any particular minimum Android version? Only extremely recent versions natively support classfile version 61.0, and older versions have limitations on what can be desugared to Java 6/7/8 bytecode.

[vy]

What I am proposing here should not block development: it takes 5 minutes to open a new issue or mark that a PR is not applicable to main.

Let's give it a try. How are we gonna streamline it?

[ppkarwasz]

If you want to streamline it, there is cherry-picker, but for me opening a small issue that just mentions which PR to port seems enough for now.