Skip to content

[cli] Write to stdout/stderr, allow redirection #207

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
merged 3 commits into from
Jun 7, 2018
Merged

[cli] Write to stdout/stderr, allow redirection #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b5bb72
[cli] Write to stdout/stderr, allow redirection
jimschubert Jun 3, 2018
9c1dddd
[cli] Limit logs to match prev implementation
jimschubert Jun 3, 2018
10fe8ae
Remove slf4j-simple from core project, to avoid conflicts with consum…
jimschubert Jun 4, 2018
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
13 changes: 9 additions & 4 deletions modules/openapi-generator-cli/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
<directory>src/main/resources</directory>
<filtering>true</filtering>
<excludes>
<exclude>logback.xml</exclude>
</excludes>
</resource>
</resources>
Expand Down Expand Up @@ -78,6 +77,12 @@
<groupId>org.openapitools</groupId>
<artifactId>openapi-generator</artifactId>
<version>${project.version}</version>
<exclusions>
<exclusion>
<groupId>org.slf4j</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

@jimschubert:

I've left the exclusion of slf4j-simple in the dependency declaration in openapi-generator-cli so we'd have record of this exclusion in the master branch.

For me this is similar to unnecessary or commented code, it will be very difficult to understand from the outside or for us in few weeks or months. I would remove those lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we remove this, it can result in a transient dependency pulling in the simple binding, and a warning.

<artifactId>slf4j-simple</artifactId>
</exclusion>
</exclusions>
</dependency>
<!--https://github.com/airlift/airline-->
<dependency>
Expand All @@ -91,9 +96,9 @@
<version>2.3.3</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
<version>${slf4j-version}</version>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<version>1.0.13</version>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you have picked version 1.0.13 for logback-classic?

It seems to me that this is an implementation for org.slf4j:slf4j-api:1.7.5 maybe it is compatible with the version 1.7.12 that we have defined (I guess also without any good reason)

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the version documented as the native implementation on slf4j: https://www.slf4j.org/manual.html

Copy link
Member

Choose a reason for hiding this comment

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

This page seems outdated to me... See: https://logback.qos.ch/download.html

Or on maven central:
http://mvnrepository.com/artifact/ch.qos.logback/logback-classic

Do we want to update to:
logback-classic version 1.2.3 and slf4j to it corresponding version 1.7.25. This way we will be in sync with swagger-core.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jmini this won't necessarily work. SLF4J's site is current.

Let me explain how slf4j works, as there seems to be some confusion.

slf4j provides an abstraction atop common logging frameworks. It does this via bindings that just need to exist in your classpath. It natively includes bindings to logback. This is why you don't have to add a binding jar, only the logback implementation. The logback usage is locked to logback 1.0.13 (see https://github.com/qos-ch/slf4j/blob/master/pom.xml) because it uses interfaces specific to that version. If you upgrade to a newer version of logback, you'd have to check if it is ABI compatible with 1.0.13. If it isn't, you can have exceptions thrown when code is JIT'd. That's obviously not ideal. It might just work with some luck, but I see no reason to chance it.

Similarly, if we don't exclude the simple logger binding explicitly, it opens us to the possibility of pulling this in as a transitive dependency. This simple logging package is the only one I've found that writes to stderr only, so allowing that to be included would result in a regression in this bug.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you a lot for the explanations... 1.0.13 seems to be the version that we need to use.

</dependency>
<dependency>
<groupId>org.testng</groupId>
Expand Down
32 changes: 27 additions & 5 deletions modules/openapi-generator-cli/src/main/resources/logback.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,34 @@
<?xml version="1.0" encoding="UTF-8"?>
<configuration>
<appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
<layout class="ch.qos.logback.classic.PatternLayout">
<Pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</Pattern>
</layout>
<target>System.out</target>
<encoder class="ch.qos.logback.classic.encoder.PatternLayoutEncoder">
<pattern>[%thread] %-5level %logger{36} - %msg%n</pattern>
</encoder>
<filter class="ch.qos.logback.classic.filter.LevelFilter">
<level>ERROR</level>
<onMatch>DENY</onMatch>
<onMismatch>NEUTRAL</onMismatch>
</filter>
</appender>
<logger name="io.swagger" level="debug"/>
<root level="error">
<appender name="STDERR" class="ch.qos.logback.core.ConsoleAppender">
<target>System.err</target>
<encoder class="ch.qos.logback.classic.encoder.PatternLayoutEncoder">
<pattern>[%thread] %-5level %logger{36} - %msg%n</pattern>
</encoder>
<filter class="ch.qos.logback.classic.filter.ThresholdFilter">
<level>ERROR</level>
</filter>
</appender>
<logger name="io.swagger" level="warn">
<appender-ref ref="STDOUT"/>
<appender-ref ref="STDERR"/>
</logger>
<logger name="org.openapitools" level="info">
<appender-ref ref="STDOUT"/>
<appender-ref ref="STDERR"/>
</logger>
<root level="error">
<appender-ref ref="STDERR"/>
</root>
</configuration>
5 changes: 0 additions & 5 deletions modules/openapi-generator/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,6 @@
<artifactId>slf4j-api</artifactId>
<version>${slf4j-version}</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
<version>${slf4j-version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
Expand Down
12 changes: 0 additions & 12 deletions modules/openapi-generator/src/main/resources/logback.xml
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions modules/openapi-generator/src/test/java/org/openapitools/codegen/php/PhpModelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@
import org.openapitools.codegen.CodegenProperty;
import org.openapitools.codegen.DefaultCodegen;
import org.openapitools.codegen.languages.PhpClientCodegen;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
Expand All @@ -46,7 +44,6 @@

@SuppressWarnings("static-method")
public class PhpModelTest {
private static final Logger LOGGER = LoggerFactory.getLogger(PhpModelTest.class);

@Test(description = "convert a simple php model")
public void simpleModelTest() {
Expand Down