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.

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

spring-core-5.2.0.M3.pom missing netty dependencies #23234

Closed
suztomo opened this issue Jul 4, 2019 · 8 comments
Closed

spring-core-5.2.0.M3.pom missing netty dependencies #23234

suztomo opened this issue Jul 4, 2019 · 8 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@suztomo
Copy link
Contributor

suztomo commented Jul 4, 2019

Affects: 5.2.0.M3

Would you declare dependencies in pom.xml when installing spring artifacts to a repository?


  • Spring-core 5.2.0.M3's pom.xml in Spring milestone repository does not have Netty dependency.
    <dependencies>
      <dependency>
        <groupId>org.springframework</groupId>
        <artifactId>spring-jcl</artifactId>
        <version>5.2.0.M3</version>
        <scope>compile</scope>
      </dependency>
    </dependencies>
    
  • Spring-core 5.1.8-RELEASE's pom.xml published in Maven Central has netty dependency with optional: true.
    ...
    <dependency>
      <groupId>io.netty</groupId>
      <artifactId>netty-buffer</artifactId>
      <version>4.1.36.Final</version>
      <scope>compile</scope>
      <optional>true</optional>
    </dependency>
    ...
    

Context

Although these optional dependencies of a transitive dependency are only used in compilation time of spring-core and not automatically retrieved by users' build tools, declaring optional dependencies in pom.xml is helpful for the following reasons:

For static analysis tools

I develop a tool to check missing dependency. The tool reads pom.xml of Maven artifacts. The tool detected spring-core 5.2.0.M3 does not have proper dependencies declared in pom.xml.

For human

Declaring these dependencies serves as human-readable documentation. For example some of the classes in spring-core use io.netty.buffer.ByteBufAllocator. Declaring optional dependency in spring-core's pom.xml helps its users to pick up correct Maven artifact to make the classes work.


With the reasons above, would you declare dependencies in pom.xml when installing to a repository?

Note that spring-core and Netty are just one example pair of this issue. There are many other dependencies that are not shown in spring-core 5.2.0.M3's pom.xml, and probably other artifacts like spring-context.

@suztomo
Copy link
Contributor Author

suztomo commented Jul 12, 2019

Thank you for picking up this for 23282

@bclozel
Copy link
Member

bclozel commented Aug 13, 2019

After discussing with other team members, I've decided to not publish that information anymore in the POMs.

While looking at all the "optional" and "provided" dependencies, I've tried to map each to a particular Gradle feature variant to provide richer metadata for build systems. In almost all cases, I didn't make sense because those libraries were not the ones required to enable the supported features.

One could think that the versions are still useful, as a guidance for developers; this is also not true, as Spring Framework often supports a range of versions for dependencies. If you're looking for such information, the Spring Boot BOM is the best source of information (and you can use it as well in non Spring Boot apps).

Now you could also consider that the dependency information is useful for maintenance reasons (CVEs, bugs, etc). Again, the Spring Boot BOM (and custom version overrides in your application) are much better at this.

We'd rather not publish misleading information in our POMs, especially if it's being used by automated tools to make important decisions or provide guidance to developers.

I'm closing this issue as a result.
Thanks!

@bclozel bclozel closed this as completed Aug 13, 2019
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 13, 2019
@suztomo
Copy link
Contributor Author

suztomo commented Aug 13, 2019

I see. I agree that it should not publish wrong information.

While looking at all the "optional" and "provided" dependencies, I've tried to map each to a particular Gradle feature variant to provide richer metadata for build systems. In almost all cases, I didn't make sense because those libraries were not the ones required to enable the supported features.

Would you elaborate on this attempt(s) that gave wrong information?

@bclozel
Copy link
Member

bclozel commented Aug 14, 2019

Let's take a few examples:

  • spring-jcl has a couple of log4j and slf4j dependencies, but those are not the actual dependencies you should have to enable logging with a logging framework. Those are dependencies that we use to build the specific bridge implementations, but using a logging framework requires more than that.
  • spring-core depends on netty-buffer, but a Spring application should not depend on Netty directly, but rather on Reactor Netty. This is used for managing DataBuffer instances with Reactor Netty as a server. So this tells us more about a particular spring-webflux use case, rather than something that is actually linked with spring-core itself
  • spring-core depends on a few reactive libraries, including Kotlin coroutines, reactor, RxJava 1 and 2. Those are not dependencies that you should necessarily have on classpath or as direct dependencies. In fact, a library or a driver can bring/use of one of those and spring-core provides an adapter for bridging between various reactive-streams implementations. So again, not a spring-core concern nor a dependency that you should worry about at that level
  • in Spring Framework modules, we're using a various combination of Servlet versions. While the Framework itself is compatible with Servlet 3.1+, some modules might build specific support for Servlet 4 features. Reading the versions of those optional dependencies is just misleading.
  • supporting various servlet containers means that we need to have several of those on classpath. We have to introduce dependency exclusions to avoid clashes (especially for API specs). Again, those are misleading because a Spring application should not have to do that.
  • now about the javax specs: they do not list the dependencies you need for a specific feature (you need the actual implementations, without our custom exclusions). The versions we're using do not show necessarily the minimum version supported by Spring, nor the advised one.
  • Spring Framework is sometimes depending on "-all.jar" variants for historical reasons, and doing so in an application or library is not advised. Arguably we should not do that in our build and this is something we should fix. But in the meantime this is incorrect/misleading information we're publishing

Taking a step back, Spring Framework is built to integrate with many technologies and libraries, and deriving sensible metadata from our build is really hard, if not impossible.

@suztomo
Copy link
Contributor Author

suztomo commented Aug 14, 2019

Thank you for good explanation.

On spring-jcl

but using a logging framework requires more than that

I think this "more than that" meant a logging backend such as logback-classic and its configuration.

In my opinion, declaring optional dependency to slf4j and log4j in pom.xml is still not wrong information. Spring-jcl's logging bridge's role is to forward logs to log4j or slf4j. Picking up logging backend or setting up logger configuration is under slf4j's usage, rather than spring-jcl's usage.

My memo on spring-jcl: https://github.com/suztomo/spring-framework-i23234/tree/master/demo-spring-jcl

On spring-core depends on netty-buffer

Your point makes sense; spring-core is not supposed to be used independently with netty-buffer.
You suppose users to depend, say, spring-boot-starter-webflux, which has transitive dependencies on spring-core and netty-buffer. Users are not supposed to depend directly on spring-core and netty-buffer. Having optional netty-buffer dependency in pom.xml is wrong information in terms of this intention.

My memo on spring-webflux: https://github.com/suztomo/spring-framework-i23234/tree/master/demo-spring-core-netty

(Continuing to check your points on servlet...)

@suztomo
Copy link
Contributor Author

suztomo commented Aug 15, 2019

On servlet versions

I picked up examples from spring-web depending on Servlet 3.1 (spring-web:5.1.9.RELEASE) and spring-webflux on Servlet 4 (spring-webflux:5.1.9.RELEASE). These optional dependencies reflect your explanation below quite well:

While the Framework itself is compatible with Servlet 3.1+, some modules might build specific
support for Servlet 4 features.

I don't see having these optional dependencies in published pom.xml misleading.

My memo: https://github.com/suztomo/spring-framework-i23234#servlet-versions-in-spring-framework

On version clashes

Optional dependencies do not cause clashes. Maven adds these optional dependencies to a class path only when it is project's direct dependencies. For example spring-web's pom.xml declaring optional dependency to servlet-api 3.1.0 (as it does in 5.1.9.RELEASE) does not have any effect in a Spring application's project. Users do not need to write exclusions for servlet-api.

My memo: https://github.com/suztomo/spring-framework-i23234#servlet-and-clashes

On "-all.jar"

I agree that having specific dependencies is good thing (regardless of this issue). I only find "netty-all" in spring-web. Created PR: https://github.com/spring-projects/spring-framework/pull/23465/files . I hope this helps.

(Continuing to check your points on javax specs...)

@suztomo
Copy link
Contributor Author

suztomo commented Aug 19, 2019

On javax spec

The version of a specification used to compile a library indicates that the project supports the
specification of that version, (without specifying other earlier or later versions).
I don't see this is wrong information, even if the library can work with other versions of
specification.


These are my feedback on the decision of not adding optional dependencies in published pom.xml.
Anyway, thank you for sharing the background of the decision.

@bclozel
Copy link
Member

bclozel commented Aug 19, 2019

Thanks for your feedback @suztomo - we've discussed that point during a team call today.
I've created #23486 to track this particular decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

3 participants