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

split project into api/impl modules #18

Merged
merged 2 commits into from
May 17, 2019
Merged

Conversation

lukasj
Copy link
Member

@lukasj lukasj commented May 2, 2019

refactor the workspace into two modules:

  • api-only module (artifactId: gmbal-api-only) - contains only API classes + ObjectRegistrationManager/ObjectRegistrationManagerNOPImpl copied from pfl-tf project + altered implementation of Named interface, so the class dependency tree is cut off early
  • impl module (artifactId: gmbal) - same as gmbal is today (includes gmbal-api-only artifact content without classes from pfl-tf project)

in terms of OSGi, this supersedes #17
in terms of JDK 11, both artifacts do have Automatic-Module-Name in manifest set to 'gmbal'. I'm thinking about adding module-info to gmbal-api-only artifact and keep Automatic-Module-Name in the implementation artifact but prefer to do it in an extra step, if you're OK with it.

within the context of Metro project, this should fix #14 and gmbal should conform to its functional spec again. At least when it comes to the packaging part.

lukasj added 2 commits May 2, 2019 21:47
Signed-off-by: Lukas Jungmann <[email protected]>
</executions>
</plugin>

<plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really mean to build the sources jar on every build?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes because impl jar depends on these sources - it unzips them to temporary folder so they can be recompiled with appropriate classpath

Copy link
Contributor

Choose a reason for hiding this comment

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

So the impl jar doesn't simply reference the api-only jar? Wouldn't it be cleaner to use the shade or assembly plugin, rather than recompiling the sources?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that simple reference to api-only jar without the need of shade or assembly plugin would be cleaner solution. Unfortunately the class dependency tree of the ManagedObjectManager goes through ObjectRegistrationManager->Named->TimerFactory to some ~30+ other files from pfl-tf. While I could add this particular method to the interface and replace the class argument with an interface to remove the dependency on the impl package, to lower the class tree to ~16 classes, it is still too much, IMO.

I personally hate using the shade plugin - while it provides some useful functionality, 95% of it can be done by assembly plugin; another drawback of this plugin is that it may transform runtime project object model in an unpredictable way (ie mvn help:effective-pom may become useless)

Assembly is good candidate but generating OSGi headers becomes complicated (unless they're written by hand).

I personally prefer recompilation, which is not that big overhead in this particular project, just to make sure everything is correct. Alternative I can come up with is to just repackage required class files instead of doing full recompilation, WDYT?

Btw: in an ideal world, there would be just a dependency from impl to api jar and no repackaging (aka split package problems on JDK9+) but this would very likely require (possibly non-trivial) changes in downstream projects

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is a bit of a mess.

If the additional classes for the impl are in the same package, the dependency idea will indeed cause problems.

OK, so this is a legacy issue, and I don't think repackaging (which isn't standard in Maven anyway) is a real improvement, so let's go with your approach.

@lukasj
Copy link
Member Author

lukasj commented May 2, 2019

btw: I used mvn -U -C clean install -Poss-release -Ddoclint=none to verify everything is built correctly on both JDK 8 and 11 (since I did not want to pollute this with javadoc fixes from #16 )

@lukasj
Copy link
Member Author

lukasj commented May 2, 2019

I quickly tested these changes in metro-jax-ws and metro-wsit projects and results look OK in both projects

@russgold russgold merged commit efc721a into eclipse-ee4j:master May 17, 2019
@lukasj lukasj deleted the split branch June 14, 2019 15:07
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.

API should not force users to redistribute pfl timers and other dependencies
2 participants