[cli] Write to stdout/stderr, allow redirection #207
Conversation
Previously, slf4j-simple from generator core was being used. This writes to only a single stream (STDERR) and is confusing from a CLI tooling perspective. This consumes logback in CLI, and excludes core's slf4j-simple dependency. This allows us to define multiple appenders, one for STDOUT and one for STDERR. WARN messages and lower are written to STDOUT. ERROR is written to STDERR.
|
@jimschubert thanks for the PR. I'll review shortly. |
|
I did a test with Would it be possible to change the log level to restore the previous output? We can then document which file to change to enable more output. |
|
@wing328 I'll look at the default levels. Previously, they were whatever the simple plugin defaulted to, not what was in logback.xml (which isn't really in use in the core package). |
|
@wing328 I've fixed this. The previous root logger was |
|
Does it make sense to have In my opinion the core should only depend from the slf4j api and let consumers (cli, other...) define the implementation they want to use ( |
|
Should I just remove it? We can't run core on it's own, and without it we would have the noop logger. I also think the logback.xml in the core project can be removed. It's a little presumptuous that the consumer will use logback, and if they're creating a fat jar it most likely needs to be removed anyway. |
|
I did not look in details into it, I have the feeling that the unit tests from the core might need to have a logger. We might just need to change the scope of |
|
@jmini When you say tests "might need to have a logger" do you mean that some tests may be instantiating a logger and outputting logs rather than doing a println? I think we can just change those to println rather than pulling in an additional dependency. After all, logging isn't really a common thing to do in tests. slf4j doesn't need a binding defined in a module (it defaults to I think later this evening, I'll remove the extra dependency and check tests for use of loggers. |
…er logger implementations
|
@jmini I've removed slf4j-simple from the core project completely. There was a single test which instantiated (but did not use) a logger. This removal results in a single warning in log output as I'd expect: This is acceptable as it doesn't pull an unnecessary artifact, and doesn't open us to the potential of having conflicting logger implementations for additional tools moving forward. 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. |
| @@ -78,6 +77,12 @@ | |||
| <groupId>org.openapitools</groupId> | |||
| <artifactId>openapi-generator</artifactId> | |||
| <version>${project.version}</version> | |||
| <exclusions> | |||
| <exclusion> | |||
| <groupId>org.slf4j</groupId> | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If we remove this, it can result in a transient dependency pulling in the simple binding, and a warning.
I consider this change as not really user friendly. Personally, when I run a unit test, I do not use the log output to debug or understand what is happening. But I have the feeling that other developers in this project do use logs. The logger is used when a unit test is executed, not inside
But we can also do it your way: remove it completely and restore it when developers complains about not seeing the logs and some SLF4J warnings when they run unit tests. |
| <version>${slf4j-version}</version> | ||
| <groupId>ch.qos.logback</groupId> | ||
| <artifactId>logback-classic</artifactId> | ||
| <version>1.0.13</version> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I used the version documented as the native implementation on slf4j: https://www.slf4j.org/manual.html
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Thank you a lot for the explanations... 1.0.13 seems to be the version that we need to use.
|
@jmini if you run tests on this branch, you should still see a lot of logs. It seems that most of what people would use during testing is output using My assumptions with testing are that people are unit testing, and are therefore focusing on a small amount of code. If things are so complex that a contributor has to rely on log messages to understand the code flow during that test, I think it's on us to reduce that complexity rather than focus on logging as a use case for testing. Enabling the simple logger binding for the test scope would be hiding that issue. I'm not against it, just very cautious about the reasoning. |
|
About logger implementation in tests, I am OK with the fact that we do not provide one. Personally I do not need it. I just consider that having one is more user friendly than having this when a unit test is executed: As @wing328 told, this is something we can document:
|
|
I did anther test with ./bin/ruby-petstore.sh and the result looks good. If no more feedback/question, I'll merge this PR tomorrow (Thur) |
* master: Add 'unblu inc.' to company list (OpenAPITools#246) put company list in alphabetical order (OpenAPITools#244) [jaxrs-spec] generate spec file (yaml) correctly (OpenAPITools#243) [C++] Adjust the names (script, sample folder, generator) to lang option (OpenAPITools#220) Add GMO Pepabo to company list (OpenAPITools#242) [Spring] Add apiFirst option (OpenAPITools#184) [cli] Write to stdout/stderr, allow redirection (OpenAPITools#207) [JAVA][Client] New object instead of null for empty POST request (OpenAPITools#98) Make yaml serialization deterministic (OpenAPITools#233) Add syntax highlighting to migration guide (OpenAPITools#237) Fix shippable badge (OpenAPITools#232) update company list (OpenAPITools#227) Fix OpenAPITools#210: [Ada] Update the code generator for required and optional parameters (OpenAPITools#211) Delete unused methods in DefaultCodegen (OpenAPITools#209) add note about maven plugins (OpenAPITools#216) add raiffeisen to company list (OpenAPITools#223) add a remark about homebrew installatio (OpenAPITools#217)
It makes sense that error messages should be written to STDERR and all others should be written to STDOUT (as shown in OpenAPITools#207). However, it would be convenient to parse the debugging output when the relevant flags are set. This change will disable logging to STDOUT and redirect all log messages to STDERR when any of the debug flags are set. (Resolves OpenAPITools#473)
It makes sense that error messages should be written to STDERR and all others should be written to STDOUT (as shown in #207). However, it would be convenient to parse the debugging output when the relevant flags are set. This change will disable logging to STDOUT and redirect all log messages to STDERR when any of the debug flags are set. (Resolves #473)
It makes sense that error messages should be written to STDERR and all others should be written to STDOUT (as shown in OpenAPITools#207). However, it would be convenient to parse the debugging output when the relevant flags are set. This change will disable logging to STDOUT and redirect all log messages to STDERR when any of the debug flags are set. (Resolves OpenAPITools#473)
Previously, slf4j-simple from generator core was being used. This writes
to only a single stream (STDERR) and is confusing from a CLI tooling
perspective.
This consumes logback in CLI, and excludes core's slf4j-simple
dependency. This allows us to define multiple appenders, one for STDOUT
and one for STDERR.
WARN messages and lower are written to STDOUT. ERROR is written to
STDERR.
See #186
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\.master.