[Core][Projects] Get packaging types from build plugins #569

kjsmita6 · 2022-02-07T20:54:48Z

This is an improved version of #538. Instead of creating an extension point, the project will now look through all of its build/extension plugins, open the jar files, and find/parse components.xml (as suggested by @laeubi ).

Two things I am unsure of:

I'm not super knowledgeable in Maven/m2e internals. I know enough about it, but not as much as might be required. I tested this code with my own product's custom packaging types and it was able to load all of them into the pom editor from components.xml. I'm not sure if everyone has it set up the same way. If not, this probably won't work.
I don't think m2e can use any plexus XML parsing code/if there is a way to parse components.xml already. If there is I can remove my own parsing code in favor of an existing one.

Again, please let me know if anything can be changed/improved. I am happy to have feedback and fix whatever is necessary. This is something we really need in our product so I would really like to have it included in m2e.

laeubi · 2022-02-08T05:32:44Z

1. I'm not sure if everyone has it set up the same way. If not, this probably won't work.

I think it don't has to work for everyone, we can improve it later if there are other means of defining this. Also with #546 we might be able to leverage other techniques if full extension support is available.

2. I don't think m2e can use any plexus XML parsing code/if there is a way to parse components.xml already.

I think standard java xml parsing is fine and might even be more flexible

laeubi

I think this is fine, just one minor adjustment.

org.eclipse.m2e.editor/src/org/eclipse/m2e/editor/pom/OverviewPage.java

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/project/registry/MavenProjectFacade.java

mickaelistria

It's great progress.
However, all that should be moved to Overview page as it's not really useful as part of the IMavenProjectFacade in general and we prefer keeping such important API minimal and more focused.

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/project/registry/MavenProjectFacade.java

mickaelistria · 2022-02-08T08:32:12Z

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/project/registry/MavenProjectFacade.java

@@ -59,6 +74,12 @@

  public static final String PROP_CONFIGURATORS = MavenProjectFacade.class.getName() + "/configurators";

+  private static final String COMPONENTS_PATH = "META-INF/plexus/components.xml";


I'm annoyed I didn't find constant in Plexus or Maven for that...

I even don't found the place where it is parsed ...

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/project/registry/MavenProjectFacade.java

org.eclipse.m2e.core/src/org/eclipse/m2e/core/project/IMavenProjectFacade.java

laeubi · 2022-02-08T08:50:00Z

@mickaelistria I think we should include this in 1.20, there is not much to be left, it is just a bit moving things here or there ... and it is local enough to have low risk for regression.

laeubi · 2022-02-08T08:52:39Z

@kjsmita6 do you think you can move the code to the OverviewPage and update your PR as of today?

mickaelistria · 2022-02-08T08:54:30Z

It's not ready and I don't want to wait further with the release. I see a bad trend of the release scope growing everyday. At some point, if we want 2.0 to happen, we need to make 1.20 highest priority.
If we wait for this one to be complete, then another interesting change will come up, and we'll then wait for this other interesting change and so on. It's already been so for the 3 last weeks. We don't have 3 more weeks to waste if we want 1.20 in next SimRel.

laeubi · 2022-02-08T08:56:48Z

If we wait for this one to be complete, then another interesting change will come up, and we'll then wait for this other interesting change and so on. It's already been so for the 3 last weeks. We don't have 3 more weeks to waste if we want 1.20 in next SimRel.

Didn't we say we don't want to be coupled to SimRel time-schedule? :-)
I don't say we should include other poping up but this one.

akurtakov · 2022-02-08T09:00:48Z

We don't want to be coupled as there are many other things to release at end of cycle thus Mickael tried to release it earlier but it ended up with endless feature growth. If 1.20 don't make it 2022-03 there is no point in even releasing it as 2022-06 will have 2.0 and many(most?) users will not get 1.20 at all.

laeubi · 2022-02-08T09:10:50Z

as 2022-06 will have 2.0

Sorry of being pessimistic but I don't see any guarantee that we will have 2.0 in 2022-06 as its completely unclear how far we got so maybe 2022-06 will still ship 1.20, that's why I have completed some important features (especially for supporting pom-dependencies) and those where people otherwise would complain (conflicting mappings).

And I don't see any 'endless' growth' there is a clear decrease in PRs targeting for 1.20 and waiting for this where a user has actually invest time for a completely different solution to came up with a important improvement for his usecase sound fair and waiting for at least another 3 Months until it bubbles up in the IDE seems not very profitable.

akurtakov · 2022-02-08T09:14:55Z

2022-06 should ship 2.0 - it's a matter of getting rid of all old stuff that's planned and get 2.0 out. Doable even in April IMHO. Any feature addition that don't make it should be for 2.1,... It's important to deliver faster rather than prolong releases.

laeubi · 2022-02-08T09:19:51Z

It's important to deliver faster rather than prolong releases.

But that's impossible with current eclipse-rules ... anyways I don't get why we repel first-time contributors due to delay their contribution because of a unclear "feeling" delaying the 1.20 at one or two days won't harm if we simply agree not including anything else.

If someone is eager to start right now, one could simply do it on a branch and we merge it in after the release, this change is local enough to be not affected by any API changes.

akurtakov · 2022-02-08T09:32:07Z

It's important to deliver faster rather than prolong releases.

But that's impossible with current eclipse-rules ... anyways I don't get why we repel first-time contributors due to delay their contribution because of a unclear "feeling" delaying the 1.20 at one or two days won't harm if we simply agree not including anything else.

It's fully possible and what should have been done. Mickael asked for 1.20 release January 21st - a release should have been done in the next few days. With main deliverable being done, someone else could have picked the releng had and do 1.20.1 or 1.21 in two weeks and repeated even one more time if needed. That way there would be way less pressure for simrel deliverable.
Added benefit for faster feedback from users that happen to use m2e p2 repo directly.

laeubi · 2022-02-08T09:44:11Z

Mickael asked for 1.20 release January 21st - a release should have been done in the next few days.

Yeah bust asking for a release is not enough, I asked for a planed date and it was said we should not plan a date, its a bit late to complain now that there is a date (now) but that was never communicated ... Also one should check tickets and assign milestones, the code was simply not ready at this point of time to be released,

someone else could have picked the releng had and do 1.20.1 or 1.21 in two weeks and repeated even one more time if needed.

someone sometimes aka never ... I just can strongly encourage to include this as it is clear and small and valuable and unlikely we will ever do a 1.20.1 once we started with the 2.0 stuff. Anyways of course no one is oblique to follow this advice, its just makes me sad that we often have this 'If I'm not affected I don't care' habit. For me #549 was a clear blocker as we would otherwise would get instant complains from m2e users once this is released to a wider audience. The second one was #543 what will be crucial if we want to support maven locations in the upcoming 2022-06 and I can't really find anything else that was done for the 1.20 in the meantime so its really a bit unfair to claim we are adding "endless new features" here...

laeubi · 2022-02-08T10:08:27Z

2022-06 should ship 2.0

Could you then please add the latest date we should have the 2.0 available here as a due-date?

akurtakov · 2022-02-08T10:12:55Z

2022-06 should ship 2.0

Could you then please add the latest date we should have the 2.0 available here as a due-date?

Done. May 24th is the latest date at which release should be public in order to make it to M3 as per https://wiki.eclipse.org/Category:SimRel-2022-06 . Wed is last contribution day in these weeks. Thu is aggregation. Fri is going public.

laeubi · 2022-02-08T10:18:36Z

2022-06 should ship 2.0

Could you then please add the latest date we should have the 2.0 available here as a due-date?

Done. May 24th is the latest date at which release should be public in order to make it to M3 as per https://wiki.eclipse.org/Category:SimRel-2022-06 . Wed is last contribution day in these weeks. Thu is aggregation. Fri is going public.

Thanks, I have for now set Friday for 1.20 please adjust if that seems unrealistic.

kjsmita6 · 2022-02-08T15:20:59Z

Pushed another commit to address the issues brought up. Namely:

Move all this code from MavenProjectFacade to OverviewPage as this is only a UI change and does not need to affect the underlying project structure.
Use a Set for adding packaging types to ensure there are no duplicates.

Note that I am in no rush to have this merged by the next release. We will most likely not be updating our Eclipse version until probably a few releases from now.

laeubi

Just a minor adjustment.

laeubi · 2022-02-08T15:31:00Z

org.eclipse.m2e.editor/src/org/eclipse/m2e/editor/pom/OverviewPage.java

+    packagingTypes.add("ear"); //$NON-NLS-1$
+    packagingTypes.add("pom"); //$NON-NLS-1$
+    packagingTypes.add("maven-plugin"); //$NON-NLS-1$
+    updateAvailablePackagingTypes(); // dynamically load available packging types from build plugins


I think you could simply place the code from updateAvailablePackagingTypes() right here and safe the field packagingTypes then and use a local variable instead.

Have this done but just waiting for final review from @mickaelistria so I can just push all changes at once.

Just squash your commits and push them here, so we can see the current status for this

+1 with @laeubi here, please push your progress without waiting for anyone. FWIW, I'm fine with this new patch, and would also like to see the changes suggested by @laeubi implemented. After that, I think it's find to merge as it's an isolated change bringing a great feature.

Alright, pushed my change. Everything should be here now.

mickaelistria

That's a great improvement to me! Thanks a lot.
@laeubi OK to merge?

Get packaging types from build plugins

f376d1f

laeubi requested a review from mickaelistria February 8, 2022 05:32

laeubi added this to the 1.20.0 milestone Feb 8, 2022

laeubi reviewed Feb 8, 2022

View reviewed changes

org.eclipse.m2e.editor/src/org/eclipse/m2e/editor/pom/OverviewPage.java Outdated Show resolved Hide resolved

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/project/registry/MavenProjectFacade.java Outdated Show resolved Hide resolved

mickaelistria requested changes Feb 8, 2022

View reviewed changes

mickaelistria removed this from the 1.20.0 milestone Feb 8, 2022

laeubi mentioned this pull request Feb 8, 2022

[Console] support eclipse-plugin type projects build with Tycho #572

Merged

Move to OverviewPage, some code improvements from review

355b169

laeubi requested a review from mickaelistria February 8, 2022 15:28

laeubi requested changes Feb 8, 2022

View reviewed changes

kjsmita6 mentioned this pull request Feb 9, 2022

"target" directory not marked as derived/hidden in Maven project nested in generic project #145

Open

Make packagingTypes local

41afe5a

mickaelistria approved these changes Feb 10, 2022

View reviewed changes

laeubi approved these changes Feb 10, 2022

View reviewed changes

mickaelistria merged commit bf03a75 into eclipse-m2e:master Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core][Projects] Get packaging types from build plugins #569

[Core][Projects] Get packaging types from build plugins #569

kjsmita6 commented Feb 7, 2022

laeubi commented Feb 8, 2022

laeubi left a comment

mickaelistria left a comment

mickaelistria Feb 8, 2022

laeubi Feb 8, 2022

laeubi commented Feb 8, 2022

laeubi commented Feb 8, 2022

mickaelistria commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

kjsmita6 commented Feb 8, 2022

laeubi left a comment

laeubi Feb 8, 2022

kjsmita6 Feb 9, 2022

laeubi Feb 9, 2022

mickaelistria Feb 9, 2022

kjsmita6 Feb 9, 2022

mickaelistria left a comment

		@@ -59,6 +74,12 @@

		public static final String PROP_CONFIGURATORS = MavenProjectFacade.class.getName() + "/configurators";

		private static final String COMPONENTS_PATH = "META-INF/plexus/components.xml";

[Core][Projects] Get packaging types from build plugins #569

[Core][Projects] Get packaging types from build plugins #569

Conversation

kjsmita6 commented Feb 7, 2022

laeubi commented Feb 8, 2022

laeubi left a comment

Choose a reason for hiding this comment

mickaelistria left a comment

Choose a reason for hiding this comment

mickaelistria Feb 8, 2022

Choose a reason for hiding this comment

laeubi Feb 8, 2022

Choose a reason for hiding this comment

laeubi commented Feb 8, 2022

laeubi commented Feb 8, 2022

mickaelistria commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

laeubi commented Feb 8, 2022

akurtakov commented Feb 8, 2022

laeubi commented Feb 8, 2022

kjsmita6 commented Feb 8, 2022

laeubi left a comment

Choose a reason for hiding this comment

laeubi Feb 8, 2022

Choose a reason for hiding this comment

kjsmita6 Feb 9, 2022

Choose a reason for hiding this comment

laeubi Feb 9, 2022

Choose a reason for hiding this comment

mickaelistria Feb 9, 2022

Choose a reason for hiding this comment

kjsmita6 Feb 9, 2022

Choose a reason for hiding this comment

mickaelistria left a comment

Choose a reason for hiding this comment