ctubbsii commented on PR #2155: URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2048181083
> Personally I am not a big fan of `optional` or `provided` dependencies: their role is documentation only. It affects behavior, so I would not say it's documentation only. But even if it were, there is value in documentation... in communicating intent to downstream consumers of the project. > The meaning is pretty much up to each developer to interpret. I interpret `provided` as "we don't bundle Logback, but you **need** it for Zookeeper to run". Regarding `optional` I interpret it as "some Zookeeper features only work with Logback" (a Logback appender maybe?). Maven [actually defines](https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html) the [meaning of these](https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html). I think your interpretation is pretty much correct, because it aligns with these definitions, but I don't think it's as subjective as you suggest. These definitions are what developers use to implement behavioral differences. Optional just means you have the option to exclude it and that Maven does so by default. It doesn't say anything about what behavioral changes might result or what you might require in its place, though, if you choose to exclude it. For that, you kind of have to know something about what is being excluded and why. A little understanding of Java logging frameworks like slf4j close this gap, though, and it will be clear to most users that the runtime implementation of slf4j is interchangeable, and that's why this implementation is optional. So, I don't think it'd be a problem to mark it as optional. > I think the message that should be sent to users is "Zookeeper prefers/recommends Logback, but it will work with whatever SLF4J implementation your provide him". I would say "happens to use" rather than "prefers/recommends", but yes, I think that message is basically the right message. But I think that message can be communicated with optional, and a basic understanding of Java logging frameworks. > Inside the Zookeeper project their usefulness is also somehow limited: the Maven Compiler and Surefire plugins will use them, but the Maven Assembly plugin will not package them in the `tar.gz` archive. > > So the only difference between runtime `optional` and `test` is that Logback is available at compilation time. This **is not** an advantage: if Zookeeper fails to compile without Logback, it means that it references some Logback-specific classes. This is not true. There's actually no difference here. If it's marked optional alone, but still in the default "compile" scope, it will be available at compilation time. However, if it's marked "runtime", it is not available at compilation time... only test. That's why "optional" is frequently used in conjunction with "runtime" scope. So, there's actually no difference in behavior between these... it really just comes down to communicating intent. > > **Remark**: The way Logback is treated in this PR is similar to the Jetty dependency. Jetty is included in the `tar.gz`, but there are no runtime dependencies on it. This is a strange comparison, because jetty is actually marked as provided, which puts it on the compile and runtime classpaths. So, contrary to what you just said, there actually *is* a runtime dependency for it. I would actually argue that these should just be regular compile (or runtime) dependencies, though, and not marked provided, since it's probably not expected that jetty will be provided by another project, since it's provided by this one. But that's outside the scope of this change. In any case, because ZooKeeper does actually have a compile time dependency on logback for its tests, I think marking it as a test dependency is fine, since it achieves the same thing as a runtime optional dependency, but I would still prefer runtime optional over test, because it communicates that something is being omitted from the runtime scope that may need to be added in order to achieve particular functionality (logging). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
