Re: Compiling with JDK 11 or JDK 17

[Volkan Yazıcı]

I completely support both initiatives:

1. Moving the JDK to 17 (why stay at 11?)
2. Moving `log4j-jmx-gui` to a separate repository

We have pulled a similar stunt in `log4j-tools`: it uses JDK 11, though
targets 8. We can easily move `log4j-jmx-gui` to a separate repository by
copying the project infra (README, CI, release process, etc.) from
`log4j-tools`.

On Fri, Mar 3, 2023 at 10:45 PM Piotr P. Karwasz 
wrote:

> Hi,
>
> Compiling 2.x using JDK 8 requires a lot of tricks:
>
> * Surefire scans classes using the main Maven JDK, so we must be sure
> `module-info.class` and other Java 9+ classes are not on the test
> classpath. This basically means we need to delete them before testing
> and creating them afterwards.
> * We need toolchains even if we disable tests,
> * Some Maven or compiler plugins (like Error Prone) either require
> Java 11 or are hard to configure on Java 8.
>
> That is why I would propose to bump the JDK requirement in the POM
> file to JDK 11+ (for `log4j-jpl`) and add `--release 8` everywhere it
> makes sense.
>
> For reproducibility purposes the CI and apache-release profiles would
> still need to fix a JDK (JDK 17?) for compilation and a JRE (JRE 8)
> for testing. But a casual user will be able to run the build process
> without toolchains.
>
> I have a working prototype on this branch:
>
> https://github.com/ppkarwasz/logging-log4j2/tree/java17
>
> The only problem I wasn't able to solve is to compile `log4j-jmx-gui`
> with JDK 11+: it requires `jconsole.jar` in the classpath, all JDK's
> after 8 have a module for that. I think we could move it to a separate
> repo.
>
> Remark that `log4j-api`, which uses `sun.reflect.Reflection`, compiles
> perfectly with JDK 17 and `--release 8`.
>
> Piotr
>

Re: [log4j] Spotless integration

[Volkan Yazıcı]

Thanks so much for bringing this to our attention Benoit. I agree that we
should pave the contribution road and make it as least painless as possible.

I am in favor of removing the ratchet from Spotless and doing a bing bang.
Since such a PR would practically not be reviewable, I guess ideally a
committer or PMC member should do that. Once the commit lands, we can skim
through the changes in IDEA to make sure Spotless did not plant a backdoor
or something.

One thing I am curious about is... *Do we all agree on the current Spotless
configuration?* Styling is pretty subjective and we should ideally have one
big bang. For instance, `log4j-tools` has a different Spotless configuration

than Log4j and it doesn't use a particular Spotless Java formatter (e.g.,
Palantir's), though this was a deliberate decision to make that
configuration align with the provided `.editorconfig`
.
Put another way, I would prefer a consistent Spotless+EditorConfig combo
across all Log4j projects. Such that we can ship this in `logging-parent`
and (hopefully) be done with formatting.

On Tue, Mar 7, 2023 at 8:46 AM Benoit Lacelle 
wrote:

> Hello,
>
> Following Piotr P. Karwasz  advice, I follow-up here a conversation from
> https://github.com/apache/logging-log4j2/issues/1317
>
> The main reason for my presence here is as author of Cleanthat (
> https://github.com/solven-eu/cleanthat) a Java linting engine. I'm a
> contributor in Spotless, a multi-language formatting engine (in which
> cleanthat has been integrated). I'm not a contributor in Log4J2.
>
> The issue in https://github.com/apache/logging-log4j2/issues/1317 is: some
> Log4J2 contributors has issues related to Spotless formatting checks. In
> this specific case, a user forked the repository, improved various files
> (spelling issues), but it's CI job are failing due to Spotless.
>
> In fact, Spotless has been configured with a given set of rules, and the
> ratchetFrom feature has been used not to require an initial
> massive-reformatting commit. This leads to each file requiring to be
> formatted by the first commiter (after Spotless integration).
> https://github.com/apache/logging-log4j2/issues/1317 author issue is the
> unexpected need to clean files around lines unrelated to his changes.
>
> Log4J2 team may decide to drop ratchetFrom feature, it would lead to a
> massive refactoring. I would not advise doing so.
> Log4J2 team may decide to keep ratchetFrom feature, but process all/some
> files given all/some reformatting rules (e.g. re-process all
> licenseHeaders).
> Log4J2 team should probably clarify this situation (the need to reformat
> the whole file as first committer since Spotless integrations) for
> contributors around
> https://github.com/apache/logging-log4j2/blob/2.x/CONTRIBUTING.md
>
> Cleanthat proposes a Github app to automatically fix PullRequests around
> Spotless configuration. https://github.com/marketplace/cleanthat/ It would
> help fixing such situations by cleaning automatically all files of a given
> PR (especially around lines not relevant to the PR author (e.g. fixing the
> license Header)). However, it would not cover
> https://github.com/apache/logging-log4j2/issues/1317 (as it is actually an
> issue in a fork of a fork of Log4J2 repository, and the PR has the fork as
> base, while Cleanthat Github app would cover PR with Log4J2 as base
> repository).
>
> FYI, I pushed this case to @nedtwig (Spotless author). We agreed the use of
> ratchetFrom with HEAD~31 and `fetch-depth: 32` is *very clever*.
>
> --
> Benoit Lacelle
> Java/ML/BigData Elite Engineer
> +33 6 78 83 92 66
> benoit.lace...@solven.eu
>

Re: [log4j] Spotless integration

[Piotr P. Karwasz]

Hi Benoit,

On Tue, 7 Mar 2023 at 08:46, Benoit Lacelle  wrote:
> Log4J2 team may decide to drop ratchetFrom feature, it would lead to a
> massive refactoring. I would not advise doing so.

I think this is the way we should go. Ratcheting has two disadvantages:

 * it doesn't work outside of a Git working tree (e.g. the source archive),
 * contributors need to fix formatting they didn't break.

> Cleanthat proposes a Github app to automatically fix PullRequests around
> Spotless configuration. https://github.com/marketplace/cleanthat/ It would
> help fixing such situations by cleaning automatically all files of a given
> PR (especially around lines not relevant to the PR author (e.g. fixing the
> license Header)).

I am not against using other Github applications, especially if their
code is ASL.
What does CleanThat propose more than a Github Actions step like this:

run: ./mvnw spotless:check || ./mvnw spotless:apply && git commit -m
"Spotless" && git push

?

> However, it would not cover
> https://github.com/apache/logging-log4j2/issues/1317 (as it is actually an
> issue in a fork of a fork of Log4J2 repository, and the PR has the fork as
> base, while Cleanthat Github app would cover PR with Log4J2 as base
> repository).

That is just a typo in the configuration.

> FYI, I pushed this case to @nedtwig (Spotless author). We agreed the use of
> ratchetFrom with HEAD~31 and `fetch-depth: 32` is *very clever*.

This is the only way for ratchet to work in Github Actions *without*
cloning the whole history of the repository. I think we never had more
than 16 commits pushed at the same time, so 32 should cover all cases.

Piotr