Skip to content

Conversation

@gnodet
Copy link
Contributor

@gnodet gnodet commented Nov 24, 2021

Packages not exported from maven-core:

  • o.a.m.bridge
  • o.a.m.eventspy
  • o.a.m.execution.infoproviders
  • o.a.m.lifecycle.mapping
  • o.a.m.feature

Packages not exported from dependencies

  • org.eclipse.sisu
    the above one is required for @priority, but there may be others needed, not sure.

Packages that should not be exported

  • o.a.m.rtinfo.internal

My commit only fixes o.a.m.feature, org.eclipse.sisu and o.a.m.rtinfo.internal.

Copy link
Contributor

@rmannibucau rmannibucau left a comment

Choose a reason for hiding this comment

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

For o.a.m.f it looks ok but I don't think we should export org.eclipse.sisu., it is internal details which shouldn't leak IMHO since it is not an API - and far from being one IMHO, no?

@gnodet
Copy link
Contributor Author

gnodet commented Nov 24, 2021

For o.a.m.f it looks ok but I don't think we should export org.eclipse.sisu., it is internal details which shouldn't leak IMHO since it is not an API - and far from being one IMHO, no?

Do you have a workaround for using @Priority ? or is that another wanted difference between core / build extensions ?

@rmannibucau
Copy link
Contributor

rmannibucau commented Nov 24, 2021

@gnodet no difference should be the target but guess it can be a build time metadata of sisu instead of a runtime annotation - or we do it by reflection and provide it in the bundle, but by the past we got a lot of troubles leaking internal IoC details so hope we don't go back there - not blaming, I know where you went from, just explaining the reasoning.

edit: Do we have a list of @priority usages, i didnt see any in core and since it is not exported it is not really a feature so can also lead to just use an API for that (public int priority();)

@gnodet
Copy link
Contributor Author

gnodet commented Nov 24, 2021

@gnodet no difference should be the target but guess it can be a build time metadata of sisu instead of a runtime annotation - or we do it by reflection and provide it in the bundle, but by the past we got a lot of troubles leaking internal IoC details so hope we don't go back there - not blaming, I know where you went from, just explaining the reasoning.

No problem with this idea, however anything we may need internally for maven may also be needed by users. I'd rather avoid relying on non supported apis, so maybe we could rely on javax.annotation.Priority which seems to be supported by Guice. But we have the same problem and Guice need to see the same class.

I'll investigate to see if I can come up with a solution what could bridge classloaders. A custom RankingFunction for sisu should do the trick.

@gnodet
Copy link
Contributor Author

gnodet commented Nov 25, 2021

@gnodet no difference should be the target but guess it can be a build time metadata of sisu instead of a runtime annotation - or we do it by reflection and provide it in the bundle, but by the past we got a lot of troubles leaking internal IoC details so hope we don't go back there - not blaming, I know where you went from, just explaining the reasoning.

No problem with this idea, however anything we may need internally for maven may also be needed by users. I'd rather avoid relying on non supported apis, so maybe we could rely on javax.annotation.Priority which seems to be supported by Guice. But we have the same problem and Guice need to see the same class.

Using java.annotation.Priority works because this package is already exported by maven-core, thus Sisu/Guice has visibility on this annotation inside extensions.

@gnodet
Copy link
Contributor Author

gnodet commented Nov 25, 2021

I've removed the export of the sisu package.

@gnodet gnodet merged commit 9d4b2b2 into apache:master Jan 18, 2022
@jira-importer
Copy link

Resolve #8094

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