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

test scope of guava in BOM #1084

Closed
i8r opened this issue Jul 5, 2021 · 10 comments
Closed

test scope of guava in BOM #1084

i8r opened this issue Jul 5, 2021 · 10 comments
Labels

Comments

@i8r
Copy link
Contributor

i8r commented Jul 5, 2021

Importing the bill of materials can cause problems with due to test scope of Guava.

Description

Guava is usually not used for test code only but the BOM defines the scope test in the dependencyManagement section.

Expected Behavior

I add the dependency of guava without defining a scope, I expect it to have the default scope of compile.

Actual Behavior

Guava is not present as compile scope even though it is declared as dependencies with the default scope.

Possible Fix

Move the test scope from dependencyManagement to dependencies.

Steps to Reproduce

  1. Create a maven project
  2. Import the BOM
  3. Add Guava as dependency without specifying the scope.

Context

Your Environment

  • Version used: 2.11.0
  • Link to your project: internal
@i8r i8r added the Bug label Jul 5, 2021
@whiskeysierra
Copy link
Collaborator

whiskeysierra commented Jul 5, 2021 via email

@i8r i8r mentioned this issue Jul 5, 2021
6 tasks
@i8r
Copy link
Contributor Author

i8r commented Jul 5, 2021

Not having a runtime time dependency on guava was a deliberate choice. What exactly is the problem?

Putting it in the dependencyManagement section changes the default scope when you declare Guava as dependency (and use the logbook BOM).

This can even blow up during runtime as it happen to me:

  1. Create a project that doesn't use Guava as dependency
  2. Import the BOM of logbook
  3. Import the BOM of riptide
  4. Declare the dependency of riptide-spring-boot-starter (that depends on guava but does not specify the scope explicitly as you usually don't do it because the default is compile)
  5. Starting the application results in the following error:
Failed to bind properties under 'riptide' to org.zalando.riptide.autoconfigure.RiptideProperties:

Reason: java.lang.ClassNotFoundException: com.google.common.collect.BiMap

@whiskeysierra
Copy link
Collaborator

4\. Declare the dependency of `riptide-spring-boot-starter` (that depends on guava but does not specify the scope explicitly as you usually don't do it because the default is `compile`)

Riptide does depend on guava in compile scope. See https://github.com/zalando/riptide/blob/1198e6ea5453413f442da7ce1046f68820f7db65/riptide-parent/pom.xml#L155-L159

Let's dig deeper into this issue. Just changing guava's scope to compile for all of logbook is wrong, since Logbook doesn't depend on guava.

@i8r
Copy link
Contributor Author

i8r commented Jul 5, 2021

Let me reproduce it in a minimal example.
(This is the zalando-internal build that was experiencing this issue.)

@i8r
Copy link
Contributor Author

i8r commented Jul 5, 2021

To reproduce the issue, run ./mvnw spring-boot:run for this project.

You will get

***************************
APPLICATION FAILED TO START
***************************

Description:

Failed to bind properties under 'riptide' to org.zalando.riptide.autoconfigure.RiptideProperties:

    Reason: java.lang.ClassNotFoundException: com.google.common.collect.ImmutableBiMap

Action:

Update your application's configuration

@i8r
Copy link
Contributor Author

i8r commented Jul 5, 2021

Just changing guava's scope to compile for all of logbook is wrong, since Logbook doesn't depend on guava.

By the way that is not what I am proposing but defining the test scope not in dependency management but the actual dependencies.

@whiskeysierra
Copy link
Collaborator

It's still wrong though, since Logbook doesn't need Guava at runtime. Riptide does.

@whiskeysierra
Copy link
Collaborator

I can't find anything on how Maven is supposed to behave in this situation.
From Logbook and Riptide's perspective, they declare their dependencies correctly:

  • Logbook only needs Guava for tests
  • Riptide needs Guava at runtime

Seems like Maven always chooses test over compile for conflicts like this.
Changing the order of the dependency blocks (pom import) doesn't change anything.

The only viable workaround that I see is to duplicate the guava dependency in your project.
Ideally Maven would either allow users to pick the scope or default to compile over test.

@i8r
Copy link
Contributor Author

i8r commented Jul 5, 2021

Seems like Maven always chooses test over compile for conflicts like this.

Could it be that it treats explicitly defined scope over implicit default scope? Haven't tried it though…

The only viable workaround that I see is to duplicate the guava dependency in your project.

Yes, That is exactly what I did in my project.

It's still wrong though.

I don't understand why you consider it to be wrong to define the scope differently as it doesn't change the effective dependencies of logbook. If you look at other BOMs, e.g. spring-boot-dependencies, you rarely see a scope defined.

@whiskeysierra
Copy link
Collaborator

I don't understand why you consider it to be wrong to define the scope differently as it doesn't change the effective dependencies of logbook.

Nevermind. I understood it as changing it from test to compile implicitly by removing the scope declaration in the dependency management section. Only now I realized that you opened a PR already.

whiskeysierra pushed a commit that referenced this issue Jul 5, 2021
Move test scope of guava from dependencyManagement to dependencies

Co-authored-by: Alexander Idelberger <alexander.idelberger@zalando.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants