Skip to content
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

Jump to bottom

4.x: Use System.Logger instead of JUL where applicable #7792 #8791

Merged
merged 4 commits into from
May 29, 2024
Merged

4.x: Use System.Logger instead of JUL where applicable #7792 #8791

merged 4 commits into from
May 29, 2024

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented May 23, 2024

Description

#7792

Documentation

N/A

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 23, 2024
@jbescos
4.x: Use System.Logger instead of JUL where applicable helidon-io#7792
f59a8f7 
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos jbescos requested a review from tomas-langer May 23, 2024 08:26
@jbescos jbescos self-assigned this May 23, 2024
@jbescos
Remove extra jump lines
69ecddc 
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos
Copy link
Member Author

jbescos commented May 27, 2024 •
edited
Loading

There are some leftovers where I was not able to remove because of any of the next reasons:

  1. There is a strong dependency to JUL in one public method and I cannot remove it to stay backwards compatible. 4.x: Use System.Logger instead of JUL where applicable #7792 (comment)
  2. Class requires dependencies on JUL (handlers, log records, etc)
$ grep -R 'requires\ java\.logging' --include 'module-info.java' ./
./webserver/observe/log/src/main/java/module-info.java:    requires java.logging;
./webserver/access-log/src/main/java/module-info.java:    requires java.logging;
./logging/jul/src/main/java/module-info.java:    requires java.logging;
./helidon/src/main/java/module-info.java:    requires java.logging;
./messaging/messaging/src/main/java/module-info.java:    requires java.logging;
./integrations/cdi/jta-weld/src/main/java/module-info.java:    requires java.logging;
./integrations/micrometer/micrometer/src/main/java/module-info.java:    requires java.logging;
./tracing/provider-tests/src/main/java/module-info.java:    requires java.logging;
./microprofile/openapi/src/main/java/module-info.java:    requires java.logging; // logging required for SnakeYAML logging workaround
./common/reactive/src/main/java/module-info.java:    requires java.logging;
./common/configurable/src/main/java/module-info.java:    requires java.logging;
./common/testing/junit5/src/main/java/module-info.java:    requires java.logging;

tomas-langer
tomas-langer requested changes May 28, 2024
View reviewed changes
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

Looks great, there are still a couple of problems to be fixed.
Thanks

helidon/src/main/java/module-info.java Outdated Show resolved Hide resolved
common/configurable/src/main/java/module-info.java Outdated Show resolved Hide resolved
webserver/observe/log/src/main/java/module-info.java Outdated Show resolved Hide resolved
tomas-langer
tomas-langer requested changes May 28, 2024
View reviewed changes
common/configurable/src/test/java/io/helidon/common/configurable/ThreadPoolTest.java Outdated
@@ -279,40 +276,6 @@ void testNullRejectionPolicy() {
), "rejectionPolicy is null");
}

@Test
Copy link
Member

Choose a reason for hiding this comment

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

Please return the test, it will work when you run it from Maven.

Copy link
Member

Choose a reason for hiding this comment

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

So the problem comes from compiler plugin, as the tests seem to be compile with module path.
To fix the problem, add this section to maven-compiler-plugin in pom.xml of the module:

                <executions>
                    <execution>
                        <id>default-testCompile</id>
                        <configuration>
                            <!--
                            We use JUL from tests, need this for compilation
                             -->
                            <compilerArgs>
                                <compilerArg>--add-modules</compilerArg>
                                <compilerArg>java.logging</compilerArg>
                            </compilerArgs>
                        </configuration>
                    </execution>
                </executions>

and re-introduce the test as it was

@jbescos
Comment reviews
dcf3572 
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
tomas-langer
tomas-langer requested changes May 29, 2024
View reviewed changes
common/configurable/src/test/java/io/helidon/common/configurable/ThreadPoolTest.java Outdated
@@ -279,40 +276,6 @@ void testNullRejectionPolicy() {
), "rejectionPolicy is null");
}

@Test
Copy link
Member

Choose a reason for hiding this comment

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

So the problem comes from compiler plugin, as the tests seem to be compile with module path.
To fix the problem, add this section to maven-compiler-plugin in pom.xml of the module:

                <executions>
                    <execution>
                        <id>default-testCompile</id>
                        <configuration>
                            <!--
                            We use JUL from tests, need this for compilation
                             -->
                            <compilerArgs>
                                <compilerArg>--add-modules</compilerArg>
                                <compilerArg>java.logging</compilerArg>
                            </compilerArgs>
                        </configuration>
                    </execution>
                </executions>

and re-introduce the test as it was

@jbescos jbescos force-pushed the issue7792 branch 3 times, most recently from 048a5b1 to c221473 Compare May 29, 2024 11:44
@jbescos
Revert ThreadPoolTest
b621c12 
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos jbescos merged commit cf82198 into helidon-io:main May 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomas-langer tomas-langer tomas-langer approved these changes

Assignees

@jbescos jbescos

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbescos @tomas-langer