Expose ArchetypeGenerator and ArchetypeManager #922

kwin · 2022-09-21T17:23:03Z

This closes #921

kwin · 2022-09-22T07:15:42Z

@laeubi The compilation inside Eclipse works fine and org.eclipse.m2e.core.ui list sorg.osgi.annotation.versioning_1.1.2.202109301733.jar as plug-dependency. Still Tycho cannot compile and fails with

Error:  Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:2.7.4:compile (default-compile) on project org.eclipse.m2e.core.ui: Compilation failure: Compilation failure: 
Error:  /home/runner/work/m2e-core/m2e-core/org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/wizards/MavenDependenciesWizardPage.java: 
Error:  	/*******************************************************************************
Error:  	^
Error:  The type org.osgi.annotation.versioning.Version cannot be resolved. It is indirectly referenced from required .class files
Error: [ERROR] 1 problem (1 error)
Error:  -> [Help 1]

The target file though contains the bundle org.osgi.annotation.versioning so I am puzzled why Tycho does not pick up that dependency....

laeubi · 2022-09-22T07:29:31Z

The target file though contains the bundle org.osgi.annotation.versioning so I am puzzled why Tycho does not pick up that dependency....

I think the problem is that Tycho 2.4 is used here.

mickaelistria

Just moving this is IMO not enough. We need to audit what are the real operations necessary to be exposed for clients to efficiently use the Archetype API here. Many operations in the promoted APIs can probably remain implementation-internal and should then be so.

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeGenerator.java

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/package-info.java

...lipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/archetype/ArchetypeGeneratorImpl.java

kwin · 2022-09-22T08:24:17Z

Just moving this is IMO not enough. We need to audit what are the real operations necessary to be exposed for clients to efficiently use the Archetype API here.

The reasoning is in #921. This PR obviously just contains the code changes.

mickaelistria · 2022-09-22T08:37:18Z

The reasoning is in #921.

The reasoning "it was this way before" is not viable. The API was removed in this major version because it wasn't appearing as useful to anyone involved regularly in the development of m2e. That is already a sign that the API state should be challenged and not cloned for the future of m2e. If we're to reopen an API, it can and should be a minimal one, just like if we were starting it from scratch.
What is the particular methods you're using in your code? We should just start by exposing as API and keep everything else internal until we have a good story to promote them.

kwin · 2022-09-22T08:38:41Z

#921 (comment)

The use case is triggering new project creation based on a specific artifact coordinates (https://github.com/apache/sling-ide-tooling/blob/93bcb69fe7cfad6b68d5caf838a14d3bdf887b92/eclipse/eclipse-m2e-ui/src/org/apache/sling/ide/eclipse/ui/wizards/np/AbstractNewMavenBasedSlingApplicationWizard.java#L106).

mickaelistria · 2022-09-22T08:41:29Z

So you would only need a way to run something like archetypeGenerator.createProject(Archetype, ...) ?

laeubi · 2022-09-22T09:01:30Z

@kwin I think Tycho support requires version 3.0.0 what will be released end of month for those annotations.

kwin · 2022-09-22T09:40:29Z

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeGenerator.java

+public interface ArchetypeGenerator {
+
+  /**
+   * Creates project structure using the given archetype and then imports created project(s)


This javadoc (although just copied from ArchetypeGeneratorImpl) seems to require an update, as Eclipse projects are no longer created by this method.

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeGenerator.java

HannesWell · 2022-09-28T20:07:09Z

@kwin if you rebase on top of the current master, some (maybe all) of the CI failures should vanish.

kwin · 2022-09-29T05:11:20Z

Before I rebase and adjust the tests I would like to know if you are fine with the proposed API exposure.

laeubi · 2022-10-05T07:05:38Z

@mickaelistria any more comments regarding API? I think this would be good to make a release after this so @kwin can adapt to the changes...

mickaelistria

This looks fresher and a well focused API! I just have a couple of questions/comments inline.

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeGenerator.java

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/MavenArchetype.java

Also expose necessary MavenArchetype (as the only implementation of IArchetype) This closes eclipse-m2e#921

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeManager.java

...lipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/archetype/ArchetypeGeneratorImpl.java

laeubi · 2022-10-05T15:42:11Z

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/archetype/ArchetypePlugin.java

@@ -74,8 +77,8 @@
 *
 * @author Eugene Kuleshov
 */
-@Component(service = ArchetypePlugin.class)
-public class ArchetypePlugin {
+@Component(service = {ArchetypeManager.class, ArchetypePlugin.class})


ArchetypePlugin should not be exposed as a service here

Not possible, as the ArchetypePlugin is the internal API which is used. If you would only register as ArchetypeManager (public API) you would need to cast in M2EUIPluginActivator. How do you want to get hold of this without casting?

Internal code can cast to internal Implementations but this should not be used as declared services (wich are exposed to everyone in the OSGi framework!)

you are exporting internal packages and by that exposing them to everyone in the framework anyways, but I am open to other suggestions. In case you prefer casting, what should happen if the implementation does not implement ArchetypePlugin? DS allows to replace one service implementation with another one!

Also remember that you previously registered only to the internal API ArchetypePlugin via DS, so that pattern is not something I introduced.

DS allows to replace one service implementation with another one!
This has nothing to do with DS, anyways you would be most likely wrecked then anyways. If you want be extra you can assert the type with instanceof and throw an exception otherwise...

Also remember that you previously registered only to the internal API ArchetypePlugin via DS, so that pattern is not something I introduced.

Many thing in m2e need improvements and some are just for migrating thing so we should not take this as good examples when refactoring/rework things.

You shouldn’t make this a prerequisite. It can be improved by someone else in a follow up

In Softwaredevelopment "later" often means "never" :-)

I would prefer to have a cast on the cases where it is required (that is we use no API method) as this is already done elsewhere in m2e and shows where we have an API gap...

Ok, fixed in 0306f85.

laeubi · 2022-10-06T09:01:00Z

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeManager.java

+ * @since 2.1.0 (package version 1.0.0)
+ */
+@ProviderType
+public interface ArchetypeManager {


Why is ArchetypeManager public? Should ArchetypeGenerator not be enough?

laeubi · 2022-10-06T09:02:13Z

Looks good so far, just wondering if we really need to make ArchetypeManager public?

kwin · 2022-10-06T11:00:06Z

ArchetypeManager is used e.g. in https://github.com/apache/sling-ide-tooling/blob/93bcb69fe7cfad6b68d5caf838a14d3bdf887b92/eclipse/eclipse-m2e-ui/src/org/apache/sling/ide/eclipse/ui/wizards/np/ChooseArchetypeWizardPage.java#L352 to filter archetypes exposed from any of the configured catalogs. Also it is used in https://github.com/apache/sling-ide-tooling/blob/93bcb69fe7cfad6b68d5caf838a14d3bdf887b92/eclipse/eclipse-m2e-ui/src/org/apache/sling/ide/eclipse/ui/wizards/np/ArchetypeParametersWizardPage.java#L273 to render a custom new project wizard for specific archetypes.

laeubi · 2022-10-06T11:33:01Z

ArchetypeManager is used e.g.

But all these should be m2e internal, so can we not make ArchetypeManager internal as well?

kwin · 2022-10-06T11:34:57Z

I use it outside of m2e, so I prefer using it with an API. I don't want to depend on internal API for my use case which was the original driver for this PR.

laeubi · 2022-10-06T11:45:58Z

I use it outside of m2e, so I prefer using it with an API. I don't want to depend on internal API for my use case which was the original driver for this PR.

Can you explain how you use this? Do you provide own UI wizard? The main problem with this (and why it was decided to make this "non API") is that the Archetype is a separate plugin, so exposing its classes directly has two problems:

We need to make people depend on a specific ArcheType version (that is the one embedded in m2e) and we only support it partially, e.g. some classes will throw CNF because we only embedd the bar minimum of stuff.
If we update the ArcheType version and that API changes, this is actually an API break for m2e even though nothing was changed here.

This has lead to the situation that previously m2e a very long time relied on outdated Archetype 2.x. and it was quite hard to upgrade to 3.x. The result is/was the IArcheType facade interface that exposed the bare minimum required.

Can you check if for your use-case something similar would be possible to have (read only) facade types with a reduced signature for RequiredProperty and ArchetypeCatalog?

kwin · 2022-10-06T11:47:11Z

We are going in circles here, I cannot explain better than above: #922 (comment). Have you checked the code?

laeubi · 2022-10-06T12:04:18Z

@kwin sorry I mixed that up with a previous comment from usages inside m2e.

About ChooseArchetypeWizardPage what do you think of instead Map<String, ArchetypeCatalog> getActiveArchetypeCatalogs() throws CoreException; having a method Stream<IArcheType> getArchetypes() throws CoreException? Would this be sufficient? It seems thats what actually is computed in that code there?

laeubi · 2022-10-06T12:07:28Z

About ArchetypeParametersWizardPage do you think you probably would reuse the m2e page? It also supports parameter validation through regexp now, otherwhise I think we can have an IRequiredProperty facade that wraps the maven RequiredPropertywith just the getters required here.

TIBCOeddie · 2022-10-12T16:33:40Z

Our product uses the ArchetypeGenerator and friends. Is there still time to look at something and provide comments?

laeubi · 2022-10-12T17:23:21Z

I think best would be too finish this PR and then if there are still things missing start with a new PR.

HannesWell · 2022-10-12T18:44:50Z

I think best would be too finish this PR and then if there are still things missing start with a new PR.

If it directly about what is changed in this PR I think comments should be made now. Of course it can be started again afterwards, but if possible why not consider them now but later?
If it is more an extension I agree that doing it step by step is better. :)

HannesWell · 2022-10-12T18:45:14Z

@kwin can you please rebase this change on top of the current master?

TIBCOeddie · 2022-10-12T19:40:19Z

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeGenerator.java

+
+  /**
+   * Creates a project structure using the given archetype in non-interactive mode.
+   *


please have javadoc for params, or you'll get all the javadoc warnings especially since j17+ have turned it up, and for sure it's always a pity to not have these! esp handy is what can be null, what is optional, etc.

I remember our integration had to dive into source to understand what "interactive mode" was

TIBCOeddie · 2022-10-12T19:41:52Z

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeManager.java

+   * Gets the required properties of an {@link IArchetype}.
+   *
+   * @param archetype the archetype possibly declaring required properties
+   * @param remoteArchetypeRepository the remote archetype repository, can be null


param is not in sync with actual

TIBCOeddie · 2022-10-12T19:42:15Z

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeManager.java

+   * @param archetype the archetype possibly declaring required properties
+   * @param remoteArchetypeRepository the remote archetype repository, can be null
+   * @param monitor the progress monitor, can be null
+   * @return the required properties of the archetypes, null if none is found


when your signature has a list, isn't it eclipse code guidelines to not return null and return an empty list instead?

+1, it's generally a good practice to not return null for Arrays or Collections but empty ones; not only for Eclipse but for Java in general.

Better return an Optional<...> instead then. There is a difference between an Archetype not found and having no required properties... one might even throw a CoreException if not found here ...

Better return an Optional<...> instead then.

I don't think so.

There is a difference between an Archetype not found and having no required properties..

Maybe, but that's not the goal of this method to detect that an Archetype is not found. But your comment here emphasize that documentation is ambiguous and that there are different ways to interpret none.

one might even throw a CoreException if not found here ...

CoreException is just an abstract exception in Eclipse world; creating a custom exception type would be better for an API.

Better return an Optional<...> instead then.

I don't think so.

Why? Either is is optional (because it is fine / could happen) but then it is not valid to just return an empty list if nothing is found as a user of that method then falsely assumes everything is fine and one could potentially call this archetype (what will just fail because it is not found)...

Maybe, but that's not the goal of this method to detect that an Archetype is not found. But your comment here emphasize that documentation is ambiguous and that there are different ways to interpret none.

Obviously there is a way to detect this, so either it is an error (= exception must be thrown) or a user has to be able to distinguish the cases.

mickaelistria · 2022-10-21T09:47:34Z

Why? Either is is optional (because it is fine / could happen) but then it is not valid to just return an empty list if nothing is found as a user of that method then falsely assumes everything is fine and one could potentially call this archetype (what will just fail because it is not found)...

There is a lot of literature against null collections, the most "reference" one being the "Effective Java" book, Your comment is based IMO on a dogmatic vision of API development; but experience of the whole Java (and other languages as well) community has concluded that returning null collections is more an anti-pattern than a benefit.

Obviously there is a way to detect this, so either it is an error (= exception must be thrown) or a user has to be able to distinguish the cases.

Or maybe we just don't care about distinguishing, do we? Or maybe since we have an Archetype object as parameter, then we can assume that this Archetype exists and is found already.

laeubi · 2022-10-21T09:55:33Z

There is a lot of literature against null collections,

And how does it relates to returning an empty Optional in case nothing could be computed here?

Or maybe we just don't care about distinguishing, do we? Or maybe since we have an Archetype object as parameter, then we can assume that this Archetype exists and is found already.

The parameter is just a simple container object and obviously the creator of that method has taken into account that the archetype is not present.

mickaelistria · 2022-10-21T10:13:37Z

Given the comments, it seems like the overall design of those APIs is then questionable as they come with very poor guarantee about validity and lifecycle of instances.

So the current IArchetype is not an IArchetype then, it's just a GAV which is more used as an ArchetypeRequest. If I have an IArchetype, in a good API, I expect the archetype exists. The issue is then not what to do with a non existing IArchetype, it's that the API should ensure an IArchetype maps to something existing.
In the current state, I'd suggest removing the IArchetype.of method which is just a GAV, and even deprecating the IArchetype interface; or to make the IArchetype interface host the method to query parameters and other stuff and let only the ArchetypeManager create the IArchetype instance if this one does exists.

HannesWell · 2022-11-09T21:01:15Z

What's the status here?

laeubi · 2022-12-03T19:24:38Z

@kwin please rebase and resolve conflicts.

laeubi · 2023-03-06T08:22:22Z

Closing this for now as no progress and no response, but feel fre to just reopen or provide a new PR on the top of the master branch.

laeubi requested a review from mickaelistria September 21, 2022 17:23

kwin force-pushed the feature/expose-archetype-generator branch 2 times, most recently from 34a5d55 to c0ab7c4 Compare September 21, 2022 17:25

mickaelistria requested changes Sep 22, 2022

View reviewed changes

kwin changed the title ~~Expose ArchetypeGenerator~~ Expose ArchetypeGenerator and ArchetypeManager Sep 22, 2022

kwin commented Sep 22, 2022

View reviewed changes

kwin requested a review from mickaelistria September 26, 2022 10:02

HannesWell reviewed Sep 28, 2022

View reviewed changes

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeGenerator.java Outdated Show resolved Hide resolved

mickaelistria reviewed Oct 5, 2022

View reviewed changes

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeGenerator.java Outdated Show resolved Hide resolved

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/MavenArchetype.java Outdated Show resolved Hide resolved

Expose ArchetypeGenerator and ArchetypeManager

d39e00f

Also expose necessary MavenArchetype (as the only implementation of IArchetype) This closes eclipse-m2e#921

kwin force-pushed the feature/expose-archetype-generator branch from ca61f5a to d39e00f Compare October 5, 2022 15:36

laeubi reviewed Oct 5, 2022

View reviewed changes

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/archetype/ArchetypeManager.java Outdated Show resolved Hide resolved

laeubi reviewed Oct 5, 2022

View reviewed changes

...lipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/archetype/ArchetypeGeneratorImpl.java Outdated Show resolved Hide resolved

laeubi reviewed Oct 5, 2022

View reviewed changes

kwin added 3 commits October 5, 2022 17:59

feedback from PR incorporated

c39ab6b

do no longer expose updateLocalCatalog(...)

50971b8

Don't register via DS for internal API ArchetypePlugin

0306f85

laeubi reviewed Oct 6, 2022

View reviewed changes

TIBCOeddie reviewed Oct 12, 2022

View reviewed changes

kjsmita6 mentioned this pull request Nov 29, 2022

ArchetypeGenerator javadoc improvements #1100

Merged

laeubi closed this Mar 6, 2023

Expose ArchetypeGenerator and ArchetypeManager #922

Expose ArchetypeGenerator and ArchetypeManager #922

Conversation

kwin commented Sep 21, 2022

kwin commented Sep 22, 2022 • edited Loading

laeubi commented Sep 22, 2022

mickaelistria left a comment

Choose a reason for hiding this comment

kwin commented Sep 22, 2022

mickaelistria commented Sep 22, 2022

kwin commented Sep 22, 2022

mickaelistria commented Sep 22, 2022

laeubi commented Sep 22, 2022

kwin Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

HannesWell commented Sep 28, 2022

kwin commented Sep 29, 2022

laeubi commented Oct 5, 2022

mickaelistria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwin Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwin Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi commented Oct 6, 2022

kwin commented Oct 6, 2022

laeubi commented Oct 6, 2022

kwin commented Oct 6, 2022

laeubi commented Oct 6, 2022

kwin commented Oct 6, 2022

laeubi commented Oct 6, 2022

laeubi commented Oct 6, 2022

TIBCOeddie commented Oct 12, 2022

laeubi commented Oct 12, 2022

HannesWell commented Oct 12, 2022

HannesWell commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelistria commented Oct 21, 2022

laeubi commented Oct 21, 2022

mickaelistria commented Oct 21, 2022

HannesWell commented Nov 9, 2022

laeubi commented Dec 3, 2022

laeubi commented Mar 6, 2023

kwin commented Sep 22, 2022 •

edited

Loading

kwin Sep 22, 2022 •

edited

Loading

kwin Oct 5, 2022 •

edited

Loading

kwin Oct 5, 2022 •

edited

Loading