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

[MRESOLVER-570] Remove excessive strictness of OSGi dependency metadata #506

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

HannesWell
Copy link
Contributor

@HannesWell HannesWell commented Jun 19, 2024

This is a forward port of #505 to resolver-2.x/master.


  • Mark optional Maven dependencies as optional in OSGi metadata
  • Widen version range for org.slf4j.spi package to [1.7,3)
  • Move bnd configuration section from execution to plugin configuration in order to simplify extensions in sub-modules.

Fixes MRESOLVER-570

@cstamas cstamas requested a review from gnodet June 19, 2024 14:20
@HannesWell HannesWell force-pushed the maven-resolver-2.x-570 branch from c710ae1 to 1b208a2 Compare June 19, 2024 15:03
@gnodet
Copy link
Contributor

gnodet commented Jun 20, 2024

@HannesWell Same comment as #505 (comment)
I wonder if we should lower the import range to [2.0,2.1) for intra project imports... ?

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

@HannesWell Same comment as #505 (comment)
I wonder if we should lower the import range to [2.0,2.1) for intra project imports... ?

- Mark optional Maven dependencies as optional in OSGi metadata
@HannesWell HannesWell force-pushed the maven-resolver-2.x-570 branch from 711341d to b9f539b Compare June 21, 2024 11:55
@HannesWell
Copy link
Contributor Author

For resolver-2x this change is actually simpler than for 1.9.x since maven-resolver-impl does not reference Sisu, Guice and org.slf4j.spi in its code anymore. The classes Slf4jLoggerFactory and AetherModule have been dropped for resolver-2.0.

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

LGTM, however, optional imports should be minimised when possible.

@gnodet gnodet merged commit 2393568 into apache:master Jun 25, 2024
5 checks passed
@HannesWell HannesWell deleted the maven-resolver-2.x-570 branch June 25, 2024 20:39
@HannesWell
Copy link
Contributor Author

Thank you for your review and submitting this.

LGTM, however, optional imports should be minimised when possible.

In general yes. But most of the time problems with it arise if the code is not really prepared to handle the absence of a bundle, since this is not that easy. But since the Maven dependencies are also marked as optional and I assume the code is prepared to handle the absence correctly respectively the use-cases are designed accordingly I think it should be fine.

@gnodet gnodet added this to the 2.0.0 milestone Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants