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

Rename plug-in o.e.m2e.pde to o.e.m2e.pde.target #653

Merged
merged 1 commit into from
May 16, 2022

Conversation

HannesWell
Copy link
Contributor

In the process of adding the o.e.m2e.pde.connector we had a discussion if multiple functionalities of M2E associated with PDE should reside in the same plug-in (i.e. o.e.m2e.pde) or in dedicated plug-ins. We decided for the latter. As a consequence the plug-in o.e.m2e.pde has actually a too general name because it only contains the Maven TargetPlatform location and is not intended to contain more.

With this PR I propose to rename the plug-in org.eclipse.m2e.pde to org.eclipse.m2e.pde.target and to do the same for its only package.
A major release of M2E is IMHO a good opportunity to perform such renaming, even tough this plug-in does not contain public API, so renaming is actually not a problem at all.
To avoid installation-problems during updates I added a p2-instruction (p2.inf) to ensure the old plug-in is uninstalled (I will test it to be absolutely sure this works before this is submitted).

@laeubi, @mickaelistria what's your opinions about this?

The plug-in o.e.m2e.pde.ui should then probably be renamed with this PR as well.

@laeubi
Copy link
Member

laeubi commented Apr 19, 2022

If you thing its worth the effort we can do that. I'm just curious if we should only have one o.e.m2e.pde.uithen instead of one ui per bundle? as we have a ne 2.x release now uninstalling the "old" bundle might even not be necessary.

@mickaelistria
Copy link
Contributor

FWIW, I don't think it's particularly a good idea to multiply bundles in small chunks of functionality by default. Unless there is a need for such a small grain, it's IMO a non-profitable effort. But I don't have deep enough knowledge about the needs and reasons and even features to know whether this applies here.

@laeubi
Copy link
Member

laeubi commented Apr 19, 2022

The main point here is that we not simply want to squash everything into one big piece just because it does anything with PDE.

For example @merks might want to use the m2e-pde-target support for Oomph as well it would be good to have this separated, so it could be easier reused. But of course more bundles are not generally better, sadly PDE itself is not very well separated an thus for example we can't reuse m2e / pde impl for target locations in Tycho but need a matching implementation...

@mickaelistria
Copy link
Contributor

If it's worthy for you (it's not for me, I don't use Oomph), please feel free to split at your own convenience; I'm just warning that this should be done with a guarantee of some ROI, because it has a long term cost (more maintenance, eg versions bumps) that is higher than keeping everything in the same bundle.

@HannesWell
Copy link
Contributor Author

If you thing its worth the effort we can do that. I'm just curious if we should only have one o.e.m2e.pde.uithen instead of one ui per bundle? as we have a ne 2.x release now uninstalling the "old" bundle might even not be necessary.

I do think its worth this relatively small effort. :) The earlier mistakes are corrected the better. And with the 2.0 major I think it is the best opportunity.

I'm just curious if we should only have one o.e.m2e.pde.uithen instead of one ui per bundle?

With your and Mickael's arguments for/against a split of pde.connector and pde.target a dedicated ui plug-in per topic is too much and I think the suggested situation is fine (pde.target, pde.connector and pde.ui).

Regarding if this should be split: My first impression was also to have only one m2e.pde bundle. But I think Christoph's point is valid so my vote would be +-0. I only then insist on more fine grained names if we have fine grained content. :)

as we have a ne 2.x release now uninstalling the "old" bundle might even not be necessary.

I don't think p2 distinguishes between micro, minor, major version bumps. But depending on whats the installation root the update could succeed without uninstalling the old plug-in. But it could then happen that both versions are installed. But I will test this.

@laeubi
Copy link
Member

laeubi commented Apr 20, 2022

With your and Mickael's arguments for/against a split of pde.connector and pde.target a dedicated ui plug-in per topic is too much and I think the suggested situation is fine (pde.target, pde.connector and pde.ui).

I just thought about it a bit and think if we split we should have:

  • pde.target
  • pde.target.ui
  • pde.target.feature
  • pde.connector
  • pde.connector.feature
  • pde.feature (including pde.target.feature and pde.connector.feature)

we might simply keep pde bundle for general purpose items but I'm not sure if there are any yet.

Even though this will add even more bundles/features this will give the user the opportunity to selectively (un)install a feature if he do not like it at all and others to depend only on the required pieces. Also for the update-case if we only change in specific parts one only needs to update less items.

@HannesWell
Copy link
Contributor Author

Even though this will add even more bundles/features this will give the user the opportunity to selectively (un)install a feature if he do not like it at all and others to depend only on the required pieces. Also for the update-case if we only change in specific parts one only needs to update less items.

To be honest I think this is way too much and too fine grained. IMHO we don't have to provide the user a possibility to control the presence of every feature (in the sense of ability) of m2e.pde via the Update-Manager. As long as one can disable or simply not use a functionality one does not want its fine to me. You always have to by the whole package, even if you just want one item.
Of course we always have to balance but that proposal, for me, goes too far in one direction.
I would also be fine to merge m2e.pde.target and m2e.connector so that we only have m2e.pde and m2e.pde.ui, but I think the aspect to reuse pde.target in Oomph and maybe even in Tycho one day is a valid reason to have it in a dedicated plug-in.

Maybe we should then only have m2e.pde (where all other future pde related code goes) and m2e.pde.target plus ui.
Or we simply have only one m2e.pde plus m2e.pde.ui for now and rework m2e.pde.target if it is really used by another project (then also providing some API). Since everything is internal this could happen at any time.

@HannesWell
Copy link
Contributor Author

Besides that I have tested the uninstallation of the old m2e.pde plugin and it looks like it is properly uninstalled even without the p2-instruction. So I removed the p2.inf file.
Furthermore I also added a target element to the packages in m2e.pde.ui that contain target related classes.

So unless we decide to have another bundling this PR is ready.

@HannesWell HannesWell marked this pull request as ready for review April 20, 2022 23:54
@laeubi
Copy link
Member

laeubi commented Apr 21, 2022

To be honest I think this is way too much and too fine grained. IMHO we don't have to provide the user a possibility to control the presence of every feature (in the sense of ability) of m2e.pde via the Update-Manager.

Well that's how Features are supposed to work, otherwise we do not gain much with the maintain from a users point of view ... Also it would help in reuse the functionality on other places, otherwise we could also just use one big m2e bundle ;-)

There was recently a discussion on BNDTools because m2e-connector seem to conflict with the bnd-tools one and one users uninstalled the whole pde feature as a workaround what of course will also include the target support. Also with the nex leminx editor there where users that are very unhappy and unistalled that specific feature, so its not that uncommon and users are aware that the can do so.

@HannesWell
Copy link
Contributor Author

HannesWell commented Apr 23, 2022

To be honest I think this is way too much and too fine grained. IMHO we don't have to provide the user a possibility to control the presence of every feature (in the sense of ability) of m2e.pde via the Update-Manager.

Well that's how Features are supposed to work, otherwise we do not gain much with the maintain from a users point of view ... Also it would help in reuse the functionality on other places, otherwise we could also just use one big m2e bundle ;-)

Having a separate plug-in m2e.pde.target would already allow to reuse it (which I assume could be very useful).
But I don't think users gain much if the target-functionality and the connector can be (un-)installed independently. If one of both is not of interest it is simply not used. I don't really see the return of invest Mickael mentioned her.

And since this is a question of balancing there is not one right solution, so it is arguable why we draw the line exactly where we draw it and not a little bit more left or right. In the end we have to come to a compromise.

There was recently a discussion on BNDTools because m2e-connector seem to conflict with the bnd-tools one and one users uninstalled the whole pde feature as a workaround what of course will also include the target support. Also with the nex leminx editor there where users that are very unhappy and unistalled that specific feature, so its not that uncommon and users are aware that the can do so.

Problems due to conflicts are of course bad. But wouldn't it be better if M2E detects such conflicts (e.g. two connectors for the same plug-in/goal?) and would handle it (e.g. by prompting the user which one to use) in general for all connectors.
This would IMHO lead to a far more better user experience than to enable a user to uninstall the pde.connecter after he/she has found out that the pde.connector causes problems. And haven't you implemented something in this regard a while ago?

@laeubi
Copy link
Member

laeubi commented Apr 23, 2022

Yes I enabled a conflict resolution. Still renaming just the bundle without having a feature for this "target feature" does not add much value, so why not keeping it as-is if we do not add any user-value with renaming it? :-)

@HannesWell
Copy link
Contributor Author

My intention with this change is not to add (big) user-value. My only goal was to make the name of the plug-in more precise respectively to fit its name it better into the changed environment (of additional m2e.pde plug-ins). That's the only little value this change adds, its just a 'clean-up'. :)

@HannesWell
Copy link
Contributor Author

How are we going to complete this PR?
@laeubi I assume I will not make you happy with the approach I'm but IMHO it is the suitable way to go for now and I would like to achive consensus (even if not everybody is ecstatic about this change).

If there is a stronger need to refactor some parts of the code into dedicated plug-ins we should be able to do this at any time since these plug-ins don't have a public API. And for installation P2 seems to take care that all plug-ins contained in the feature are installed. Only those that create Plug-in based Products will notice that, but I assume usually Features are used to build Products, aren't they?

Instead of having a o.e.m2e.pde.target and o.e.m2e.pde.connector the latter could also be named o.e.m2e.pde and possible future code that for the m2e-pde bridge could go there as well. Or we can do that if such code really arrives (as said above it should not be a big deal to change those plug-in names). A third option would be to merge all non-ui m2e-pde code into a o.e.m2e.pde plug-in and move the target-code into a dedicated plug-in as soon as there is a real need for it.

@laeubi
Copy link
Member

laeubi commented May 14, 2022

@HannesWell if you think its good, just goo ahead, I just wanted to give some insights how we could structure things so they are useful for adopters and users if we are already doing renaming things.
If that do nut suffice I'm fine and I literally don't care much about how things are named as long as one has a chance to find them later on :-)

@HannesWell
Copy link
Contributor Author

@HannesWell if you think its good, just goo ahead, I just wanted to give some insights how we could structure things so they are useful for adopters and users if we are already doing renaming things. If that do nut suffice I'm fine and I literally don't care much about how things are named as long as one has a chance to find them later on :-)

OK, good. :) I have to admit that unsuitable names bother me more than they should for being just names. But usually they can't be change without breaking a lot of code so I have to live with that.

@mickaelistria do you have any more comments on this one?

@HannesWell
Copy link
Contributor Author

Since no further objections were raised, I'm going to submit this now.

@HannesWell HannesWell merged commit 810c8e7 into eclipse-m2e:master May 16, 2022
@HannesWell HannesWell deleted the pde.target branch May 16, 2022 18:16
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