Bugzilla – Full Text Bug Listing
| Summary: | [ui] No update proposed when there are repository with errors | ||
|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Pascal Rapicault <pascal> |
| Component: | p2 | Assignee: | Ian Bull <irbull> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | irbull, john.arthorne, Lars.Vogel, tjwatson |
| Version: | 3.7 | Flags: | tjwatson:
pmc_approved+
pascal: review+ |
| Target Milestone: | Juno RC1 | ||
| Hardware: | PC | ||
| OS: | Mac OS X - Carbon (unsup.) | ||
| Whiteboard: | |||
|
Description
Pascal Rapicault
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. *** Bug 363913 has been marked as a duplicate of this bug. *** 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. 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 (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 (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. Pascal, just hold off a second on the review. I've got an update coming. 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? I've reviewed this change with Ian and it is good to go. +1, Ian please merge the change to integration for RC1, otherwise we will have RC1 with your previous solution before you reverted it. Fixed and merged into Integration. Thanks everyone! 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. +1 to the comment of John, searching for broken repos was very annoying. |