This Bugzilla instance is deprecated, and most Eclipse projects now use GitHub or Eclipse GitLab. Please see the deprecation plan for details.
Bug 349628 - [ui] No update proposed when there are repository with errors
Summary: [ui] No update proposed when there are repository with errors
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: 3.7   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 major (vote)
Target Milestone: Juno RC1   Edit
Assignee: Ian Bull CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 363913 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-06-16 17:29 EDT by Pascal Rapicault CLA
Modified: 2012-05-15 17:21 EDT (History)
4 users (show)

See Also:
tjwatson: pmc_approved+
pascal: review+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pascal Rapicault CLA 2011-06-16 17:29:17 EDT
When my installation refers to repositories that no longer exist, then searching for update (Help > Check for Updates) will fail to present any updates.
Comment 1 Thomas Watson CLA 2011-08-31 10:34:21 EDT
We discussed this on the Equinox call a couple of weeks ago.  Is there any more information?  Is this a regression from 3.6?  Do we have a fix?  We have had our last scheduled 3.7.1 build so I think it is too late to consider doing anything in 3.7.1.  Deferring to 3.7.2.
Comment 2 Matthew Piggott CLA 2011-11-16 10:14:03 EST
*** Bug 363913 has been marked as a duplicate of this bug. ***
Comment 3 Ian Bull CLA 2012-01-23 13:06:32 EST
I don't think this will get done for 3.7.2. But we should fix this for 3.8. I'm going to put it on the M5 milestone to see what's involved.
Comment 4 Ian Bull CLA 2012-05-14 18:21:01 EDT
I have a fix for this. The fix works by introducing a flag on the repository load job that suppresses repository load errors. This flag is set during the background loading of the repositories during the check for update.

I have released this so I can test this with tonights build. However, this fix will require a code review.

Let's not get too excited about this yet because there are two problems.

1. This requires a code review, and there is an argument to be made for the current behaviour. For example, with this new behaviour, the current state of your IDE (which bundles we choose to install) is now dependent on what repositories are reachable.  If I 'check for updates' while repository A is unreachable, and you check while it is reachable, we may end up with different configurations (Let's say A has an IU that we would like to install, but we could live without).

We have strived for consistent installs in the past. The same argument is made about composite repos and their children.

2. This fix is actually an API change :-(.  It's not a breaking API change, but it does introduce a new public constant on a public (API) class.  

If anybody objects to either 1 or 2 we will have to revert this (and possibly re-think the solution).

In the mean-time I've released the fix so we can test it.  Pascal, can you code review this? Tom, can you comment on the new API?

http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=8a16090df20423751452e26b96863507dfd20514
Comment 5 Thomas Watson CLA 2012-05-15 09:26:03 EDT
(In reply to comment #4)
> I have a fix for this. The fix works by introducing a flag on the repository
> load job that suppresses repository load errors. This flag is set during the
> background loading of the repositories during the check for update.
> 
> I have released this so I can test this with tonights build. However, this fix
> will require a code review.
> 
> Let's not get too excited about this yet because there are two problems.
> 
> 1. This requires a code review, and there is an argument to be made for the
> current behaviour. For example, with this new behaviour, the current state of
> your IDE (which bundles we choose to install) is now dependent on what
> repositories are reachable.  If I 'check for updates' while repository A is
> unreachable, and you check while it is reachable, we may end up with different
> configurations (Let's say A has an IU that we would like to install, but we
> could live without).
> 
> We have strived for consistent installs in the past. The same argument is made
> about composite repos and their children.

Did we ever get an answer to my comment 2?  Was this a change in behavior in some release?  I think the ideal solution would involve posting a warning to the user and asking if they would like to fix the repository location, ignore for this operation or fail the operation.

> 
> 2. This fix is actually an API change :-(.  It's not a breaking API change, but
> it does introduce a new public constant on a public (API) class.  

I would hold of on introducing new API since I am not convinced it is the correct final solution.  I could convinced to add the new API and behavior change if we could determine the an unintended behavioral change occurred between the 3.6 and 3.7 release which started the update failure to occur when there are repository errors. 

> 
> If anybody objects to either 1 or 2 we will have to revert this (and possibly
> re-think the solution).
> 
> In the mean-time I've released the fix so we can test it.  Pascal, can you code
> review this? Tom, can you comment on the new API?
> 
> http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=8a16090df20423751452e26b96863507dfd20514
Comment 6 Ian Bull CLA 2012-05-15 12:03:10 EDT
(In reply to comment #5)
> Did we ever get an answer to my comment 2?  Was this a change in behavior in
> some release?  I think the ideal solution would involve posting a warning to
> the user and asking if they would like to fix the repository location, ignore
> for this operation or fail the operation.

In 3.6.2 the behaviour was as follows: 1. reported the error (an error dialog would come up), 2. it still continued with the check (and show a dialog with the result of that). This is a little goofy since there are two modal dialogs now, and sometimes it's the lower one that needs to be dismissed first.

The commit that resulted in this change was

http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=a2edaff672f5478364a56e7498e3f8f4054acd2b

I can easily introduce the old behaviour (error dialog + continue to check for updates). Let me see if I can do it cleanly without any new API Constant.
Comment 7 Ian Bull CLA 2012-05-15 12:32:22 EDT
Pascal, just hold off a second on the review. I've got an update coming.
Comment 8 Ian Bull CLA 2012-05-15 12:56:19 EDT
Ok, here is the new commit:

http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=cb3694589d14417cbbce60197f5b787dae4d04f5

I've reverted the change I made yesterday and released this one instead.  With this change, we now have the same behaviour as 3.6.2 (without the IAE).  The user will see an error dialog of all repositories that failed to load, but the update check will continue.

I've scoped this behaviour to only happen during the 'preload repository hanlder' which is used for background loading of jobs. In order to do this, I needed access to a private method from a superclass (API Class). I increased the visibility to protected. This shouldn't be an API change since the class is marked as noextend. -- That is, only people who are already breaking the API contact will see the new API.

I'll double-check that API tools has the same view on that.

Pascal, if you get a chance, can you review this change?
Comment 9 Pascal Rapicault CLA 2012-05-15 16:11:09 EDT
I've reviewed this change with Ian and it is good to go.
Comment 10 Thomas Watson CLA 2012-05-15 16:21:23 EDT
+1, Ian please merge the change to integration for RC1, otherwise we will have RC1 with your previous solution before you reverted it.
Comment 11 Ian Bull CLA 2012-05-15 16:27:56 EDT
Fixed and merged into Integration. Thanks everyone!
Comment 12 John Arthorne CLA 2012-05-15 17:15:24 EDT
Yay, thanks Ian! This personally annoyed me because I always have some random repos in my list that aren't reachable. IMO if it can find updates I don't care if some of my other repos weren't reachable at the time.
Comment 13 Lars Vogel CLA 2012-05-15 17:21:16 EDT
+1 to the comment of John, searching for broken repos was very annoying.