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

Overhaul the feature editor #26

Closed
laeubi opened this issue Apr 11, 2022 · 15 comments
Closed

Overhaul the feature editor #26

laeubi opened this issue Apr 11, 2022 · 15 comments
Labels
wontfix This will not be worked on

Comments

@laeubi
Copy link
Contributor

laeubi commented Apr 11, 2022

The usage and meaning of features has recently cause a lot of confusion, for that reason I'd like to suggest an overhaul of the feature-editor to reflect today realities:

  1. Included Plugins/Included Features Tab: according to @mickaelistria these are actually obsolete and could be expressed by "Dependencies" in a much more flexible and consistent way
  2. Dependencies are actually misleading, as P2 always installs all required transitive dependencies (@merks @mickaelistria correct?) it is rather useless to specify anything here as a "Plugin Dependency", what one actually does here is express a "Restriction" or "Additional Requirement" that can't be expressed in a Plugin itself (e.g. a fragment))
  3. [2.7.1][regression] Neither raw version nor format was specified eclipse-tycho/tycho#876 shows that the information is actually even duplicated between 1+2 most probably as a side-effect of the "Compute" / "Recompute when feature plugins change" function
  4. The help states that

The Dependencies page lists all Required Features and Plug-ins that must be present in the product before the feature can be installed. If any of these pre-requisites are missing, the feature will not be installed.

what is actually not true according to @mickaelistria, also @HannesWell recently has added support for PDE behaving the same as P2 when using "adding additional requirements" to a product launch.

My suggestion would be the following to make this more clear and consistent to the user:

  1. remove the "Included Plugins/Included Features Tab" and convert any feature.xml upon saving into equivalent import plugin/feature items
  2. remove "Compute" / "Recompute when feature plugins change"
  3. rename "Dependencies" to "Content" or "Features and Bundles"
  4. Update the Help to reflect how features actually work

There is only one thing currently not possible, that is to express an optional/os dependent feature but that information seems to be duplicated as well as a feature already carries information about OS and a like, as well as a plugin can define a platform filter and express if it is to be packaed/unpackked.

@merks
Copy link
Contributor

merks commented Apr 11, 2022

I don't think any realities have changed. Plugin dependencies have always been respected even before p2. I don't think this should be arbitrarily changed now because I don't see any benefit for removing things or for a forced migration to something different for any existing users....

I should also point out that the statement you say isn't true is in fact true. You need only look at the IU published from a feature and see that in fact requirements are expressed that make the statement true. E.g., here:

https://download.eclipse.org/oomph/archive/reports/download.eclipse.org/releases/2022-03/202203161000/index/org.eclipse.equinox.p2.core.feature.feature.group_1.6.1300.v20220223-1131.html

You can see that the feature requires a specific version of sat4j.

  <required namespace="org.eclipse.equinox.p2.iu" name="org.sat4j.core" range="[2.3.5.v201308161310,2.3.5.v201308161310] "/>

Michael decided to remove the explicit constraint to use that specific version, but that's not the same as the use of includes by features is in need of a general overhaul for everyone using PDE.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 11, 2022

I don't say that they are not expressed, but expressing them explicitly as a "dependency" seems superflous if the are installed anyway even if I not add them there. So from a user point of view, these are not "Dependencies" but more "additional items" as sat4j will be installed anyways. And given I have imported it with a proper version range whats the point then to list it?

Michael decided to remove the explicit constraint to use that specific version, but that's not the same as the use of includes

Sure, but it was argued that it is obsolete anyways and of no use and then I think we should make this more clear instead of letting people add superfluous things...

@merks
Copy link
Contributor

merks commented Apr 11, 2022

You did say a statement that is true is not true and it seems to me conclusions for actions to be taken were drawn from that.

If something is superfluous then it need not be used. With an include, I can force a specific known version to be installed and I can force combinations of specific things to be installed. One may or may not want that so one can choose to use this aspect or not. But you are suggesting to "remove the "Included Plugins/Included Features Tab" and convert any feature.xml upon saving into equivalent import plugin/feature items". No, do not do that! Features really do work they way they are described, and people have been using this for decades. So I don't think anything you are suggesting is an improvement but rather a disruptive change that will lead to new confusion.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 11, 2022

@mickaelistria wrote:

I'd like to get rid of those, as transitve deps do get installed transitively anyway without having them included in features

so this for me reads as if they are superfluous. If not, why don't you suggested there "No, do not do that! Features really do work they way they are described, and people have been using this for decades."?

Maybe people are using features wrong for decades? At the moment the feature editor yells at me I should declare dependencies, while whenever I ask for adding missing ones I got told that they are actually not needed... (and actually we now remove already added ones).

@merks
Copy link
Contributor

merks commented Apr 11, 2022

Consider that perhaps you may be misunderstanding the use and over assuming what's superfluous. Let me repeat the use case. Inclusion allows me to specify installing a combination of specific versions of other features and bundles. This use case is not superfluous, but rather something you can exploit to the effect you wish to achieve at your personal discretion.

Even in this case of the change to p2.core's feature which prompted this issue, not all the plug-in includes were removed (nor was there a discussion with a boarder audience if this change was a good idea or not). So in this case, should all the plugin includes be changed to imports? Should the imports have exact version range compatibility and can a version number that will be inserted during the build? Does that actually work now? Will that then be entirely equivalent (produce the same IU requirements) to what we have now? Will it then be a giant no-op?

Note that the SDK workspace uses plugin inclusion > 1000 times:

image

If you continue to feel strongly that maybe everyone, including everyone developing all the sub-projects of the Eclipse top-level project, is using this wrongly, you should get agreement to force everyone to change from what they do now to what you think is better for them.

@vogella
Copy link
Contributor

vogella commented Apr 11, 2022

Multiple of my clients use the dependencies tab to document explicitely the requirement of a feature.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 11, 2022

This use case is not superfluous, but rather something you can exploit to the effect you wish to achieve at your personal discretion.

I don't say that the use case is superfluous, I just said that it seems duplicated functionality and it is not clear to the user what he should use, just try the following:

<includes id="com.test.base.feature" version="1.0.0"/>

versus

   <requires>
      <import feature="com.test.base.feature" match="exact" version="1.0.0"/>
   </requires>

what produces the same p2 data.so it seems you can express the same thing with two different ways so one might get the feeling that one of those is not needed.

Even in this case of the change to p2.core's feature which prompted this issue, not all the plug-in includes were removed (nor was there a discussion with a boarder audience if this change was a good idea or not).

But why it was added then at the first place? Obviously because the user feels he has to declare it and just adds it because it probably will be needed... and that's what I think should be changed, helping people making the right decisions and and adding problematic things just because they did it for 20years ...

Multiple of my clients use the dependencies tab to document explicitely the requirement of a feature.

They might not get what is desired, as P2 will probably install multiple versions of the same thing then if not carefully managing these versions.

@merks
Copy link
Contributor

merks commented Apr 11, 2022

You didn't answer all my questions, so let me answer the most significant one for you. This does not work.

image

It produces this requirement which is not what I need/want:

    <required namespace='org.eclipse.equinox.p2.iu' name='org.eclipse.oomph.setup' range='0.0.0'/>

When I hit recompute in the editor, the list is very big (which I don't want nor need) and even then I can't express what I do need even like this:

image

I.e., the version qualifier is not known so I can't express a 'perfect' range that is correct.

So I don't think there is an equivalent way to express what I need and want.

I've always understood inclusion to be like containment (as in EMF) while dependencies are like EMF cross references to other containment trees and I'm very happy to use inclusion where I can specify version 0.0.0 and have everything work exactly like I want it to work.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 11, 2022

It produces this requirement which is not what I need/want:

You should simply add it without a version/match rule if you want similar things to using version 0.0.0 what actually got expanded at build time to the highest version available and not to the highest compatible version to your bundle, what most probably is not what you want but only works because you mostly use one update-site per release...

When I hit recompute in the editor, the list is very big (which I don't want nor need)

That's what I noted in my initial request, this function is not very useful and produces weird results, but well it worked for 20 years that way I think... so we probably should simply not change it and not use it ;-)

Anyways, as always wasted time to try improvising PDE so I'm closing this.

@laeubi laeubi closed this as completed Apr 11, 2022
@merks
Copy link
Contributor

merks commented Apr 11, 2022

That's what I noted in my initial request, this function is not very useful and produces weird results, but well it worked for 20 years that way I think... so we probably should simply not change it and not use it ;-)

Here we are talking about the Dependencies tab which you advocated as a replacement for the "Included Plugins" and "Include Features" tabs. I'm pretty sure the button just computes the transitive closure of the dependencies and I'm pretty sure it does so correctly. But I don't need that so I don't use that. I didn't suggest it was weird or wrong. I also didn't see concretely what you were suggesting to improve with respect that the "Compute" button's action...

Going back to the main point, your premise in this issue was to remove some tabs and to convert inclusion to imports automatically. So I spent time explaining how they're different things for different purposes and how they actually work to make it clear that the functionality is not superfluous. Do I waste my time when I explain that? I hope not. Though you continue to suggest that expanding 0.0.0 with the current version is somehow not doing exactly what I want, when in fact it does exactly what I want and need; I want to define a group of my own bundles (most of which are generally singletons by design) as features so that they can be installed as a group. I want to be able to update that feature such that it will update that entire set of bundles...

There are definitely cases (in Oomph for example) where I do not want to include a bundle in a feature such that my feature can be installed with whatever version of that bundle is available from the Platform or from ECF.

Anyways, as always wasted time to try improvising PDE so I'm closing this.

This part is really uncalled for, especially when you're the one who recently suggest that all generalizations are false. :-P The underlying premise here is that your idea was sound to start with and that stupid, ignorant people who simply don't want change, and don't see the wisdom of your idea, wasted your time. That's not fair and is not the case...

@Bananeweizen
Copy link
Contributor

You cannot just propose to convert dependencies and includes into one thing, they are completely different, and it would break a lot of currently used installation flows:

  • By including features/plugins I can easily check whether my product is self-contained for delivery. Not every feature is available from an update site, remember this is not just about eclipse.org projects. Not every computer is even connected to the Internet (in Asia, our Eclipse products are installed on completely offline machines from a locally hosted update site).
  • When using lots of Eclipse components, you get into version conflicts easily. They are typically easier to solve with included features/plugins than with dependencies. I'm not particularly sure about the actual reason, but my simple understanding says that dependencies are often more loosely specified than included components, because included components can get their exact singleton version during the build. Not sure if that makes sense, but it fits to our experience, that includes lead to less surprises at customers.

@HannesWell
Copy link
Member

I've always understood inclusion to be like containment (as in EMF) while dependencies are like EMF cross references to other containment trees and I'm very happy to use inclusion where I can specify version 0.0.0 and have everything work exactly like I want it to work.

That's how I understand that too and although there is a functional overlap, for me "Included Plug-ins/Features" and "Dependencies" have a different semantics (but it's hard to tell exactly). But there is also a hard technical difference when building a p2-repo from features using Tycho. If tycho-p2-repository::includeAllDependencies is false (the default) only (transitively) included Plug-ins and Features are added. If it is true all required dependencies are added too. If you would only use dependencies, that repo would be empty by default (but of course the default could change).

Therefore I think the distinction between Included Plug-ins/Features and Dependencies in general is fine. And I think it is up to the creator of the Feature to craft a good feature. Nevertheless there could be better documentation/guidelines/best-practices when to use which capability and what are the pros and cons.

2. remove  "Compute" / "Recompute when feature plugins change"

This is IMHO a good suggestion because it is really not necessary and only encourages people to do thinks that are not necessary and could lead to conflicts. In the past I was also wondering if I should use that until I learned that it is not necessary.
Therefore removing this with a clear announcement with an explanation seems beneficial to me.
So I think it is worth reopening this issue to address that, isn't it?

@laeubi
Copy link
Contributor Author

laeubi commented Apr 12, 2022

Here we are talking about the Dependencies tab which you advocated as a replacement for the "Included Plugins" and "Include Features" tabs.

Correct, because the do very very similar things, but I never advocated to change the persistence (feature.xml) so one could simply improve this one to e.g. have a checkbox "resolve to build version" that then produces a plugin, or if not set creates an import, because that's the main difference here and what has bugged @mickaelistria that the first is resolved only once at build time and the later at install time, requiring the feature to be rebuild and redeploy on every dependency change!

You cannot just propose to convert dependencies and includes into one thing, they are completely different

See above, and looking at the P2 metadata they are not completely different, the only difference is compile vs install time if you choose the emptyVersion.

This part is really uncalled for, especially when you're the one who recently suggest that all generalizations are false. :-P

Sure including this statement ;-)

The underlying premise here is that your idea was sound to start with and that stupid, ignorant people who simply don't want change, and don't see the wisdom of your idea, wasted your time.

Sorry if it sounds rude, but its only me to blame here that I wasted my time even though I know better and I don't want to spend more time if I don't see we are converge to a solution so I don't want to waste your time as well.

That's not fair and is not the case...

One can discuss/improve any idea (and maybe come to the conclusion that it was not that good as initially intended) but can't compete with

  • We have done it always that way
  • So many other do it that way
  • I don't need it / like it

But there is also a hard technical difference when building a p2-repo from features using Tycho. If tycho-p2-repository::includeAllDependencies

I haven't tried it out but from my experience/testing that's not the case, because on the P2 level Tycho can't distinguish between both. includeAllDependencies has nothing to do with feature Dependencies (as noted above I think the help-page and wording is confusing), tycho just includes all requirements (that was all plugin/import actually are converted into), and then includes the dependencies of those (e.g. requirements of a bundle or a feature) and then starts over again until there is no new content.

So I think it is worth reopening this issue to address that, isn't it?

If you (or anyone else) think any idea/suggestion is worth to be further discussed, I think its more suitable to create a new more smaller scoped issue, maybe my initial request was just not well shaped, tries to archive to much at once and I was not able to express what the benefits are in a larger context.

@Bananeweizen
Copy link
Contributor

:+1 for a separate issue about removing the "Compute" button. Also for us that has been a source of problems by adding too many and (from my point of view) unclear dependencies.

@HannesWell
Copy link
Member

:+1 for a separate issue about removing the "Compute" button. Also for us that has been a source of problems by adding too many and (from my point of view) unclear dependencies.

I created #37 for this. Please express your consent or objections there.

laeubi pushed a commit to laeubi/eclipse.pde that referenced this issue Nov 2, 2023
* Update tips and tricks for 4.24 in PDE

Change-Id: Ic678657008a07e89e783b77de47f833959ab7d1b
Signed-off-by: Vikas Chandra <Vikas.Chandra@in.ibm.com>

* Updated with copyright year

Change-Id: Ia10c3fea0f3806a7b13c3b3d2938f96f6cee0e57
Signed-off-by: Vikas Chandra <Vikas.Chandra@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants