Bug 506850 - Provide option for preferring workspace bundles at all times and make it default
Summary: Provide option for preferring workspace bundles at all times and make it default
Status: VERIFIED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.7   Edit
Hardware: PC Linux
: P3 normal with 1 vote (vote)
Target Milestone: 4.7 M4   Edit
Assignee: Vikas Chandra CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
: 495233 500701 (view as bug list)
Depends on:
Blocks: 498184 507534 509159
  Show dependency tree
 
Reported: 2016-11-01 16:23 EDT by Stefan Xenos CLA
Modified: 2019-02-24 15:57 EST (History)
9 users (show)

See Also:


Attachments
Proposed option (60.72 KB, image/jpeg)
2016-11-03 05:31 EDT, Vikas Chandra CLA
no flags Details
Tentative patch - will finalise the patch if this approach is agreed on. (9.19 KB, patch)
2016-11-03 05:31 EDT, Vikas Chandra CLA
no flags Details | Diff
Couple of projects. Behavior of this changes with "Always prefer workspace" option selection (206.40 KB, application/zip)
2016-11-03 05:34 EDT, Vikas Chandra CLA
no flags Details
Updated patch - few issues still to be addressed. (9.41 KB, patch)
2016-11-03 07:45 EDT, Vikas Chandra CLA
no flags Details | Diff
Patch for testing ->removed deprecated method in last patch. (9.18 KB, patch)
2016-11-03 07:55 EDT, Vikas Chandra CLA
no flags Details | Diff
Updated fix (10.65 KB, patch)
2016-11-04 04:13 EDT, Vikas Chandra CLA
no flags Details | Diff
Clearer option name and a tooltip (2.62 KB, patch)
2016-11-23 00:45 EST, Vikas Chandra CLA
no flags Details | Diff
Snap with updated option name and updated tooltip (70.39 KB, image/jpeg)
2016-11-23 00:48 EST, Vikas Chandra CLA
no flags Details
Updated option name and tooltip (64.90 KB, image/jpeg)
2016-11-24 02:58 EST, Vikas Chandra CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Xenos CLA 2016-11-01 16:23:21 EDT
How to reproduce:

In an empty workspace, follow the steps for setting up your workspace for contributing to platform UI:

https://wiki.eclipse.org/Platform_UI/How_to_Contribute

At the end (assuming that someone has recently added and adopted a new API in git), you'll have some compiler errors. Specifically, any plugin that invokes an API from another plugin which is present in your workspace but isn't present in your build target will show up as a compilation error.

Observed:

It appears that classpath resolution is always preferring the target platform over the workspace version when the same plugin is present in both places.

Expected:

The workspace version should be preferred over the version from the target platform.
Comment 1 Stefan Xenos CLA 2016-11-01 16:26:37 EDT
+Ed, since this has implications for the Oomph setup scripts. +Lars, who may be interested in how this affects new contributors and holds the keys to PDE. +Sergey, since this bug has been a thorn in our side for some time.
Comment 2 Vikas Chandra CLA 2016-11-02 00:37:28 EDT
>> when the same plugin is present in both places.

Are the version numbers different.

If the same plugin with same version number is present in both workspace and target, the workspace plugin should be picked.
Comment 3 Ed Merks CLA 2016-11-02 01:10:35 EDT
As far as I understand it and expect it, the workspace version should always be picked regardless of any version numbers.  One thing to note is that it's often the case that simply restarting the IDE encourages PDE to update the classpath correct.  I think one reason this problem occurs is because of the large number of imports happening at once.  We do the project importing in a workspace runnable, but I believe because of the large volume, a notification is issued anyway.  I suspect that PDE gets confused by this...

This is definitely a super super annoying problem that makes it really hard to set up larger projects automatically (or manually for that matter).  What's wrong is subtle and hard to notice and goes away in the same subtle way when you restart.
Comment 4 Stefan Xenos CLA 2016-11-02 09:25:21 EDT
At Google we've been working around this by modifying the target platform to remove any plugin that's also present in the workspace, but this can take a very long time if you have hundreds of projects loaded. It cost me a good chunk of my day yesterday. :-(
Comment 5 Vikas Chandra CLA 2016-11-03 00:52:55 EDT
There have been couple of fixes in M3. Also I have fixed 1 another scenario yesterday.

>>the workspace version should always be picked regardless of any version numbers


This is no longer true. What if the version constaint for a plugin is not satisfied by workspace bundle but a target bundle?

Right now, the expected behavior is that if the version of workspace is same or more than the version in target, that version is picked else the highest version of that bundle is picked.

But still if the expectation is that the workspace bundle should be picked no matter what, a preference option could be 1 way to solve it.
Comment 6 Lars Vogel CLA 2016-11-03 01:20:09 EDT
(In reply to Vikas Chandra from comment #5)
> There have been couple of fixes in M3. Also I have fixed 1 another scenario
> yesterday.

Can you point to the bug report?

> >>the workspace version should always be picked regardless of any version numbers
> 
> 
> This is no longer true. What if the version constaint for a plugin is not
> satisfied by workspace bundle but a target bundle?

+1, of the version in the ws does not fit, the target version with a fitting version should be used.

> Right now, the expected behavior is that if the version of workspace is same
> or more than the version in target, that version is picked else the highest
> version of that bundle is picked.

I would expect that the bundle in the ws gets always priority, if it has a fitting version.
Comment 7 Vikas Chandra CLA 2016-11-03 01:37:28 EDT
>>Can you point to the bug report?

See Bug 475797  and also Bug 499486

>>I would expect that the bundle in the ws gets always priority,

I think that is the case unless the workspace bundle has a lesser version than the target bundle. I think that the expectation that workspace bundle has at least same or greater version than the target bundle is a reasonable one. But I may be wrong.

>>hard to notice and goes away in the same subtle way when you restart.


This should go away with yesterday's fix but there are few errors on cent-os platform I need to work on.
Comment 8 Ed Merks CLA 2016-11-03 01:59:07 EDT
I wonder though how is this supposed to work with singletons? I.e., when I launch and include my workspace plugin, how does that interact with this (apparently new) behavior? 

> Right now, the expected behavior is that if the version of workspace is same
> or more than the version in target, that version is picked else the highest
> version of that bundle is picked.
> 

This does not seem right to me, and especially not for singletons. I don't see how this paints a consistent picture.  One always expects the workspace plugin to be used both for launching (when it's selected) and of course when it's used for launching I expect it will be used for building...

> But still if the expectation is that the workspace bundle should be picked
> no matter what, a preference option could be 1 way to solve it.

Yes though if some behavior has changed, a preference to choose the new behavior would seem more appropriate than a preference to restore the behavior as it's been in place for the last 15 years...  But I don't fully understand how the new behavior is supposed to work with singletons.  It's clear if I launch with a bundle and it's a singleton, it must be used.  So how does that fit into this new picture?

In any case, at the moment, it seems the choice of how the bundle is resolved is somewhat random and arbitrary and subject to change after a restart but presumably that will go away?

Lars,

Be very careful of what you ask for with your comment "+1, of the version in the ws does not fit, the target version with a fitting version should be used." because a bundle in the workspace will typically have a .qualifier version so if the TP has included a feature that includes that "same" bundle, the binary version will not have a .qualifier in the version but rather a build qualifier/stamp/date.  And of course a feature includes its bundles with *exact* versions so the workspace bundle will not generally fit such exact version constraints (according to actual p2 constraint solving) and hence would not be used according to this "fit" criterion (unless "fit" is better or more loosely defined).  

I think in all cases that it would be very surprising if a singleton bundle in the workspace is not always used.  Certainly when it's the case that it's not being used, the user must be informed and must have the option to ensure that it is used.  So I think "fit" should include the fact that for singletons only one bundle with that ID can "fit" and therefore if a bundle with that ID is in the workspace that one must be the one that "fits" because it's the one I see in my workspace, the one I modify when I edit, and the one I have in the workspace presumably for the purpose of being the right "fit" in the first place.
Comment 9 Lars Vogel CLA 2016-11-03 02:05:36 EDT
(In reply to Vikas Chandra from comment #7)

> I think that is the case unless the workspace bundle has a lesser version
> than the target bundle. I think that the expectation that workspace bundle
> has at least same or greater version than the target bundle is a reasonable
> one. But I may be wrong.

I can describe at least one real scenario in which I want the workspace version, even if the version is lower:

Assume you try to triage an error and import a plug-in into the workspace. Now you set the plug-in via the history view into an older state to find the commit which created the error. This might result in a lower version of the plug-in version compared to  the target version. I would still expect the workspace version to be picked up.
Comment 10 Vikas Chandra CLA 2016-11-03 04:56:40 EDT
Right now, the best fit bundle ( and highest version bundle) is picked. So typically if workspace bundle is same or greater than target bundle, that is picked. If workspace bundle is less than the target bundle, then target bundle is picked.

>>how the bundle is resolved is somewhat random and arbitrary  & subject to >>change after a restart

As I said earlier that is a bug ( which hopefully is fixed now).

Based on discussion above, I think an option to "Always prefer workspace bundle" should be added. The only issue is whether we want default as true or false. I have most of the code ready for this change.
Comment 11 Vikas Chandra CLA 2016-11-03 05:31:06 EDT
Created attachment 265168 [details]
Proposed option
Comment 12 Vikas Chandra CLA 2016-11-03 05:31:45 EDT
Created attachment 265169 [details]
Tentative patch - will finalise the patch if this approach is agreed on.
Comment 13 Vikas Chandra CLA 2016-11-03 05:34:12 EDT
Created attachment 265170 [details]
Couple of projects. Behavior of this changes with "Always prefer workspace" option selection
Comment 14 Vikas Chandra CLA 2016-11-03 07:30:35 EDT
There are couple of issues with the patch attached. I am currently fixing all scenarios.
Comment 15 Vikas Chandra CLA 2016-11-03 07:45:22 EDT
Created attachment 265174 [details]
Updated patch - few issues still to be addressed.
Comment 16 Vikas Chandra CLA 2016-11-03 07:55:37 EDT
Created attachment 265175 [details]
Patch for testing ->removed deprecated method in last patch.
Comment 17 Stefan Xenos CLA 2016-11-03 09:36:57 EDT
> I think in all cases that it would be very surprising if a singleton bundle
> in the workspace is not always used.

I can think of two different scenarios in which you'd want different behavior.

1. I'm editing the code in one of my plugins.

In this case, I'd want the workspace version to be used - but I'd also want to know if - due to its version number - P2 would have ignored this plugin if it had been installed into the current target. So I'd want to use the workspace version but I'd *also* want an error or warning marker if the act of preferring the workspace version over the latest version made any difference. Otherwise if I were to deploy the plugin from my workspace into my target after it worked correctly in my workspace, it wouldn't be used and I wouldn't have a clear indication as to why.

2. I'm editing the dependency graph.

Maybe I'm preparing a distribution or update site and I'm trying to get all the dependency constraints right. Maybe I'm bumping all the major plugin version numbers after a big release... or maybe I'm debugging a P2 bug that can only be reproduced when P2 rejects an older version of a plugin. In these scenarios, you'd want the behavior of PDE to match the behavior of a live distribution as closely as possible, even if that means the code from the workspace is ignored.

However, in this scenario I'd also want to know if the version in my workspace is older than the one in my target since I'd never want to do this accidentally.


So IMO a preference makes sense. That way you can toggle between the two types of diagnostics. I suspect that use case 1 is much more common (since normal developers tend to outnumber releng developers), so I'd argue that "always resolve from workspace" should be the default. However, in all cases I'd want to be clearly informed if my workspace version was older than the one in the target.
Comment 18 Vikas Chandra CLA 2016-11-04 00:48:37 EDT
Based on the various feedback here and on Bug 475797, I think we can go ahead with this change. ( after some testing)

>>if my workspace version was older than the one in the target.

There was never any tooling for this one. But I think this is very important issue and warrants a separate bug.
Comment 19 Vikas Chandra CLA 2016-11-04 01:05:32 EDT
Since there are many other similar open defects ( see defects triaged in M4), I have changed the subject to specify the change that will be done to solve this and similar kind of issues.
Comment 20 Sergey Prigogin CLA 2016-11-04 01:05:47 EDT
(In reply to Stefan Xenos from comment #17)
> 2. I'm editing the dependency graph.
> 
> Maybe I'm preparing a distribution or update site and I'm trying to get all
> the dependency constraints right. Maybe I'm bumping all the major plugin
> version numbers after a big release... or maybe I'm debugging a P2 bug that
> can only be reproduced when P2 rejects an older version of a plugin. In
> these scenarios, you'd want the behavior of PDE to match the behavior of a
> live distribution as closely as possible, even if that means the code from
> the workspace is ignored.

Even this scenario does not justify compilation of workspace code against the target version of a class when the same class is present in the workspace.
Comment 21 Vikas Chandra CLA 2016-11-04 01:11:07 EDT
>>if my workspace version was older than the one in the target.

opened Bug 507015 for this issue.
Comment 22 Vikas Chandra CLA 2016-11-04 04:13:31 EDT
Created attachment 265186 [details]
Updated fix
Comment 23 Vikas Chandra CLA 2016-11-04 04:16:15 EDT
I updated the fix

1) Fix works fine for the projects attached.
2) pde.ui test cases run fine
3) Multiple version of a bundle scenario works fine
4) Default option is "always prefer workspace bundle"
5) The option is location as per "Proposed option" attachment
Comment 24 Vikas Chandra CLA 2016-11-04 04:20:51 EDT
Fixed via
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=42acf4b4b77f26a083df7eddb60e0a1ad54465a1

But I am keeping this bug open for any suggestion in option name, location in preference, tooltip ( ?? ) or any such tweaks that may be required.

I am committing this early in 4.7M4 so that any issues can be taken care of early.
Comment 25 Vikas Chandra CLA 2016-11-04 04:23:01 EDT
*** Bug 495233 has been marked as a duplicate of this bug. ***
Comment 26 Vikas Chandra CLA 2016-11-04 04:23:43 EDT
*** Bug 500701 has been marked as a duplicate of this bug. ***
Comment 27 Stefan Xenos CLA 2016-11-04 12:07:03 EDT
Regarding the scenario where you're editing the dependency graph in the workspace:

> Even this scenario does not justify compilation of workspace code against
> the target version of a class when the same class is present in the workspace.

I think it may. If you've changed your dependencies such that the workspace version of a plugin is incompatible with your target and the version number of its dependency in your workspace is such that P2 would ignore that plugin if installed, I'd want to know about it... and a workspace full of compilation errors is one way to tell the user that something's wrong.

Admittedly, it's not as good as detecting the error directly and creating a single error marker that explains the problem clearly... but it's still better than silently ignoring the error and pretending that everything is okay.

(Note: this only applies to users who are working on the dependency graph. A programmer that is writing Java code almost certainly doesn't want to see such errors.)
Comment 28 Sergey Prigogin CLA 2016-11-04 15:32:51 EDT
(In reply to Stefan Xenos from comment #27)
> Admittedly, it's not as good as detecting the error directly and creating a
> single error marker that explains the problem clearly... but it's still
> better than silently ignoring the error and pretending that everything is
> okay.

You seem to be agreeing with comment #20. It doesn't make sense to have a lame solution if we can have a good one.
Comment 30 Markus Keller CLA 2016-11-15 11:48:14 EST
Can you move the new "Always prefer workspace bundle" preference from the general "Plug-in Development" preference page to the "Target Platform" page?

All the other options on the general page are about presentation and user interaction, but not about how to build and launch from a workspace.

The "Target Platform" would be a better place for this preference, since that's where the other bundles are defined. The introductory text on that page also sets a proper context that clarifies the meaning of "Always prefer workspace bundle".

I'm not too happy with the name "Always prefer workspace bundle". What does "prefer" mean exactly, and what's the opposite? Always prefer target platform bundle? Probably not...

How about: "Workspace plug-ins override all versions from target platform"?

"&A" conflicts with "&Apply".
Comment 31 Sergey Prigogin CLA 2016-11-15 12:36:58 EST
Agree with moving the preference to the Target Platform page.

It makes sense to invert the preference and call it "Target platform takes precedence over workspace plug-ins" or something like that. The inverted preference should be off by default.
Comment 32 Markus Keller CLA 2016-11-16 05:12:10 EST
(In reply to Sergey Prigogin from comment #31)
> It makes sense to invert the preference and call it "Target platform takes
> precedence over workspace plug-ins" or something like that.

I did not suggest to invert the preference, and I don't think your proposed label would be correct. The target platform never takes precedence. In the non-default setting, plug-ins from the target platform are just not suppressed from the resolution.

> How about: "Workspace plug-ins override all versions from target platform"?

The intention of "override all versions" is to clarify what the "prefer"/"takes precendence" actually means.
Comment 33 Sergey Prigogin CLA 2016-11-16 10:16:38 EST
(In reply to Markus Keller from comment #32)
The current behavior is that the target platform does take precedence on the compile time classpath causing a lot of confusion.

Inverting the preference makes sense because the default behavior would correspond to the preference turned off. Better wording for the inverted preference is welcome.
Comment 34 Markus Keller CLA 2016-11-16 12:55:45 EST
(In reply to Sergey Prigogin from comment #33)
> The current behavior is that the target platform does take precedence on the
> compile time classpath causing a lot of confusion.

"Current" means I20161114-0355 / master, and there the current default is to revert to the old (4.5) behavior where workspace plug-ins always take precedence. I'm not opposing that default.

The new mode is not "the target platform takes precedence". It's "the target platform is just another source of plug-ins, and normal version resolution chooses plug-ins from the workspace as well as from the target platform".

> Inverting the preference makes sense because the default behavior would
> correspond to the preference turned off. Better wording for the inverted
> preference is welcome.

I agree that disabled-by-default is in general the better way to present an option. However, in this case I don't see a way to write this on a single line in a way that also tells what the alternative setting does.

The opposite of "Workspace plug-ins override all versions from target platform" is pretty clear: No plug-in source overrides plug-ins from another source, so normal OSGi resolution applies.
Comment 35 Sergey Prigogin CLA 2016-11-16 13:33:18 EST
(In reply to Sergey Prigogin from comment #33)
> The current behavior is that the target platform does take precedence on the
> compile time classpath causing a lot of confusion.

I need to clarify that by "current" behavior I meant the behavior in 4.6.1.
Comment 36 Sergey Prigogin CLA 2016-11-16 13:35:08 EST
(In reply to Markus Keller from comment #34)
> The opposite of "Workspace plug-ins override all versions from target
> platform" is pretty clear: No plug-in source overrides plug-ins from another
> source, so normal OSGi resolution applies.

OSGi resolution does not affect compile time classpath, but this option does.
Comment 37 Vikas Chandra CLA 2016-11-18 01:02:54 EST
>All the other options on the general page are about presentation and user interaction, 
>but not about how to build and launch from a workspace.

The options at "General settings for plug-in development are "general" options which are more basic in nature.  "Update stale manifest files prior to launching" doesn't  look like UI option. Based on this, I had put the option here. 

>>The opposite of "Workspace plug-ins override all versions from target platform" is
>>pretty clear: No plug-in source overrides plug-ins from another source, so normal 
>>OSGi resolution applies.

The opposite to that option could mean "Workspace plug-in doesnt override all versions from target platform" which again is confusing. It "could" ( to "some" users) mean that target platform plugin takes precedence. ( which is not the case).

>I'm not too happy with the name "Always prefer workspace bundle". What does "prefer" mean 
>exactly, and what's the opposite? Always prefer target platform bundle? Probably not...

Agree this is confusing. The opposite is "Don't always prefer workspace bundle".
The key difference is the word "always".  But the original option name is shorter. It could be expanded.

So I would think
"Workspace plug-ins always override all versions from target platform"
would work too.

And so would

"Always prefer workspace bundle to the target bundle for a plug-in".
The opposite to this would be "dont always prefer workspace bundle to the target
bundle for a plug-in"

We could probably add a tooltip on what happens on unchecking. ( to make it more clear).

As I understand, most of the users want workspace bundle to override target bundle for the same bundle id. So it would be better to have an option that is checked by default.
Comment 38 Vikas Chandra CLA 2016-11-22 01:53:02 EST
The target platform preference page is for adding, editing or removing target definition whereas this preference is more about core plugin development behavior. Putting settings of how plugins are chosen over one another is a misfit in "target platform preference page"

So I think this setting should reside in "General settings for Plug-in development".
Comment 39 Ed Merks CLA 2016-11-22 04:43:53 EST
(In reply to Vikas Chandra from comment #38)
> ...
> So I think this setting should reside in "General settings for Plug-in
> development".

I tend to agree.  The Target Platform page has mostly selection-dependent action buttons so an option that applies to all seems not so appropriate and the current layout is no conducive to adding a checkbox.

One might argue it could/should be a property of a target definition, but that's an even more complicated design feature, so I wouldn't personally suggest that.
Comment 40 Vikas Chandra CLA 2016-11-23 00:45:09 EST
Created attachment 265523 [details]
Clearer option name and a tooltip
Comment 41 Vikas Chandra CLA 2016-11-23 00:48:02 EST
Created attachment 265524 [details]
Snap with updated option name and updated tooltip
Comment 42 Vikas Chandra CLA 2016-11-23 00:49:31 EST
I think a tooltip is required to make things clearer especially in a scenario when the option is turned off.
Comment 43 Markus Keller CLA 2016-11-23 13:22:52 EST
OK, keeping the option on the general page is fine with me.

(In reply to Vikas Chandra from comment #40)
> Always prefer workspace plug-in to a target plug-in for a plug-in id

3 times "plug-in" in a row is too much. Better:
  Always prefer workspace plug-ins to target plug-ins with the same id

However, "prefer" still sounds like there could be a chance that the "preferred" plug-in is eventually not chosen (e.g. because that would be the only way to satisfy all dependency constraints). "Always" improves it a bit, but opens even more possibilities for what the alternative could be.

"Workspace plug-ins override target platform plug-ins with the same id" would solve that.


> +MainPreferencePage_AlwaysPreferWorkspaceTooltip=Turning this option off would mean that there is no specific preference in plug-in selection.\nFor a plug-in id, the best available plug-in will be chosen.

The tooltip should not assume the option is turned on. Better say:
"When disabled, all plug-in versions from workspace and target platform are being used."
Comment 44 Vikas Chandra CLA 2016-11-24 02:58:38 EST
Created attachment 265554 [details]
Updated option name and tooltip
Comment 45 Vikas Chandra CLA 2016-11-24 03:00:19 EST
Fixed option name and tooltip as per comment#43 as it looks reasonable.
via
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=a513ad20b1e0423958fe5b81a102bb0facbc8170

Also adjusted some variable names/comment etc to reflect the new option name.
Comment 46 Vikas Chandra CLA 2016-12-05 04:19:42 EST
verified on
Version: Oxygen (4.7)
Build id: I20161203-2000
Comment 47 John Ruud CLA 2016-12-05 12:52:22 EST
This is great news! Would it be feasible for this fix to be ported to Neon.3 as well (scheduled for 3/23/2017)? I am staying with Mars for plugin development for now, in order to not have to deal with working around this particular problem.
Comment 48 Vikas Chandra CLA 2016-12-06 01:13:12 EST
In Neon.2 , we have made a change and workspace bundle is always picked. (similar to Mars). See Bug 475797. I think Neon.2 would work for you.

However, if there is a need to toggle between the two modes, we can consider this for Neon.3
Comment 49 John Ruud CLA 2016-12-06 12:49:56 EST
Great - similar behavior to the Mars is all I need. I'll install Neon.2 simultaneous release once it's available, and will report back if it's not providing what I need.