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

For Maven, if a dependency with runtime and test, a compile dependency is added #2683

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

sdelamo
Copy link
Contributor

@sdelamo sdelamo commented Jan 2, 2025

if a dependency is added to the scopes runtime and test in maven. The dependency should be in compile only for maven.

We should not generate this:

<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>runtime</scope>
</dependency>
<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>test</scope>
</dependency>

But this:

<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>compile</scope>
</dependency>

@sdelamo sdelamo added type: bug Something isn't working build: maven labels Jan 2, 2025
@sdelamo sdelamo requested review from alvarosanchez and melix January 2, 2025 16:16
@sdelamo sdelamo changed the base branch from 4.8.x to 4.7.x January 2, 2025 16:23
@sdelamo sdelamo marked this pull request as ready for review January 2, 2025 16:24
@melix
Copy link
Contributor

melix commented Jan 6, 2025

I don't understand this fix. A compile dependency will make the dependency part of the transitive compile dependencies, which will leak to consumers. Having both runtime and test dependencies is more accurate, since these are really runtime only deps.

@alvarosanchez
Copy link
Member

Agree with @melix. Moreover, the extra test scope is redundant since runtime dependencies are already in the test classpath

@sdelamo
Copy link
Contributor Author

sdelamo commented Jan 7, 2025

In maven, what we are seeing is that if you have:

<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>runtime</scope>
</dependency>
<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>test</scope>
</dependency>

then there is no logback at runtime. it seems the dependency is set as test scope only.
You can see it in action in this guide: https://guides.micronaut.io/latest/micronaut-server-filter-request-maven-java.html

@alvarosanchez
Copy link
Member

Probably the latter overrides the former. The extra test scope needs to be removed

in graadle if runtime and test => runtime and test
@sdelamo
Copy link
Contributor Author

sdelamo commented Jan 7, 2025

@alvarosanchez and @melix

I have changed the PR.

For Maven

These dependencies:

<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>runtime</scope>
</dependency>
<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>test</scope>
</dependency>

get converted to:

<dependency>
  <groupId>ch.qos.logback</groupId>
  <artifactId>logback-classic</artifactId>
  <scope>runtime</scope>
</dependency>

For Gradle

These dependencies:

runtimeOnly("ch.qos.logback:logback-classic")
testImplementation("ch.qos.logback:logback-classic")

it remains the same:

runtimeOnly("ch.qos.logback:logback-classic")
testImplementation("ch.qos.logback:logback-classic")

@sdelamo sdelamo merged commit a3d5e86 into 4.7.x Jan 7, 2025
13 checks passed
@sdelamo sdelamo deleted the if-runtime-test-use-compile branch January 7, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build: maven type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants