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.

Sign up for GitHub

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

Jump to bottom

Use provided scope for jboss-logging-annotations dependency #46681

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use provided scope for jboss-logging-annotations dependency #46681

wants to merge 1 commit into from

Conversation

YassineHaouzane
Copy link

Fixes #45761

This PR adds the provided scope to the dependency jboss-logging-annotations

I also added a commit that pass the dependency jboss-logging-processor as an argument to annotationProcessorPaths.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Mar 9, 2025

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)
  • title should not start with chore/docs/feat/fix/refactor but be a proper sentence

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file labels Mar 9, 2025
@YassineHaouzane YassineHaouzane changed the title fix: provided scope jboss-logging-annotations dependency Use provided scope for jboss-logging-annotations dependency Mar 9, 2025
gsmet
gsmet reviewed Mar 10, 2025
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I added some comments.

bom/application/pom.xml Outdated
Comment on lines 6618 to 6629
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<annotationProcessorPaths>
<annotationProcessorPath>
<groupId>org.jboss.logging</groupId>
<artifactId>jboss-logging-processor</artifactId>
<version>${jboss-logging-annotations.version}</version>
</annotationProcessorPath>
</annotationProcessorPaths>
</configuration>
</plugin>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plugin configuration is not inherited from BOM so I think this should go away.

bom/application/pom.xml Outdated
Comment on lines 4768 to 4771
<dependency>
<groupId>org.jboss.logging</groupId>
<artifactId>jboss-logging-processor</artifactId>
<version>${jboss-logging-annotations.version}</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to stay around. It's used by the recent versions of the Maven compiler plugin to define the default version of the annotation processor.

bom/application/pom.xml
<groupId>org.jboss.logging</groupId>
<artifactId>jboss-logging-processor</artifactId>
<version>${jboss-logging-annotations.version}</version>
<scope>provided</scope>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually refrain from adding scopes in the BOM as it makes things less flexible (e.g. if for some reasons, you want to use another scope).

Now I'm not entirely sure using another scope makes sense in this very specific case. WDYT @dmlloyd ?

@YassineHaouzane YassineHaouzane force-pushed the fix/provided-scope-jboss-logging-dependency branch from 86bb1e5 to 51ece6e Compare March 10, 2025 17:02
@YassineHaouzane
Copy link
Author

I removed the commit that removes the jboss-logging-processor and pass it to the annotationProcessorPaths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/core area/dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't expose jboss-logging-annotations
2 participants
@YassineHaouzane @gsmet