Issue 207 - Add additional buttons für deleting and removing update sites to the dialog "Error Contacting Site" #343

erik-brangs · 2023-10-02T14:23:25Z

Here's an attempt to fix the issues discussed in the first few posts of #207 , i.e. additional buttons for disabling and removing update sites. This PR does not implement any link rewriting as discussed later in #207.

I've made an attempt to write some tests but I don't know if that's the correct way to go about it. It's also not clear to me if events are needed for the operations. ColocatedRepositoryTracker also doesn't seem to use them. I've added TODOs for that in the PR.

github-actions · 2023-10-28T17:04:02Z

Test Results

9 files ± 0 9 suites ±0 29m 39s ⏱️ +37s
2 196 tests +12 2 192 ✅ +12 4 💤 ±0 0 ❌ ±0
6 678 runs +36 6 667 ✅ +36 11 💤 ±0 0 ❌ ±0

Results for commit 8c2d65c. ± Comparison against base commit 1b21340.

♻️ This comment has been updated with latest results.

laeubi · 2023-10-28T17:40:59Z

@akurtakov @merks the build seems green so far can we integrate this?

merks · 2023-10-28T18:21:28Z

I see 4 failed builds. I don’t think they failed because of your changes though.

laeubi · 2023-10-28T18:26:02Z

Arggg... I missed the missing version bumps... @erik-brangs can you please increase the versions now?

laeubi · 2023-10-29T17:45:38Z

@erik-brangs thanks Jenkins succeeds now and I approved GH actions, can you probably squash your commits into one?

...org.eclipse.equinox.p2.ui/src/org/eclipse/equinox/internal/p2/ui/LocationNotFoundDialog.java

...x.p2.tests.ui/src/org/eclipse/equinox/p2/tests/ui/operations/LocationNotFoundDialogTest.java

laeubi · 2023-10-30T07:24:40Z

@merks for me this looks sane now, a test is added that also succeeds so maybe we should give this a try?

merks · 2023-10-30T07:58:22Z

Trying it out locally, the first thing I tried was to add a bogus site and do check for updates, which fails like this:

I realize that this PR does not try to address that case.

Just entering/choosing a bogus site fails like this:

which is also fine I suppose.

If I enter a good site, but then ask for looking in All Available Sites, it fails like this, though I can close the dialog and ignore that failure:

When I install something like this, I see exceptions in the log, but nothing in the UI about that:

and the install succeeds:

I actually have not been able to figure out when the dialog kicks in that I might test this manually. Perhaps someone can enlighten me how I can test this manually...

erik-brangs · 2023-10-30T12:51:25Z

You should see the dialog if you do the following:

Add an update site for http://download.eclipse.org/technology/m2e/releases/ via Available Software Sites
Use Install New Software with All Available Sites

I suppose there can be caching involved. I'm not sure what to delete to get a clean state.

laeubi · 2023-11-03T07:45:37Z

@erik-brangs can you verify the mentioned places by @merks so the dialog is showing there as well?

merks · 2023-11-03T08:05:41Z

Note that I don't want to block anything here. I would just be nice to see it in action...

laeubi · 2023-12-09T06:57:00Z

@erik-brangs do you plan to further work on this? Stream is open so you need to resolve the manifest conflict and rebase but this would be a good time for such changes.

erik-brangs · 2023-12-10T17:25:23Z

I will continue working on this pull request this month.

erik-brangs · 2023-12-20T14:05:54Z

I've taken another look. Here's an updated reproducer to get the dialog to show:

use a freshly extracted copy of Eclipse and start it
Add an update site for http://download.eclipse.org/technology/m2e/releases/ via Available Software Sites
Use Install New Software with All Available Sites

It seems the dialog does not appear on subsequent runs of the same Eclipse instance. I don't know why that is.

I also can't seem to get the dialog when clicking through the UI of an Eclipse produced via Run Configurations from the default Equinox predefined setup.

The stack traces differ for a downloaded Eclipse (which has Oomph)

org.eclipse.equinox.p2.core.ProvisionException: No repository found at http://download.eclipse.org/technology/m2e/releases/.
	at org.eclipse.equinox.internal.p2.repository.helpers.AbstractRepositoryManager.fail(AbstractRepositoryManager.java:409)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.eclipse.oomph.util.ReflectUtil.invokeMethod(ReflectUtil.java:119)
	at org.eclipse.oomph.p2.internal.core.CachingRepositoryManager.fail(CachingRepositoryManager.java:388)
	at org.eclipse.oomph.p2.internal.core.CachingRepositoryManager.loadRepository(CachingRepositoryManager.java:279)
	at org.eclipse.oomph.p2.internal.core.CachingRepositoryManager$Metadata.loadRepository(CachingRepositoryManager.java:520)
	at org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:110)
	at org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:105)
	at org.eclipse.equinox.p2.ui.LoadMetadataRepositoryJob.doLoad(LoadMetadataRepositoryJob.java:126)
	at org.eclipse.equinox.p2.ui.LoadMetadataRepositoryJob.runModal(LoadMetadataRepositoryJob.java:110)
	at org.eclipse.equinox.internal.p2.ui.sdk.PreloadingRepositoryHandler$1.runModal(PreloadingRepositoryHandler.java:84)
	at org.eclipse.equinox.p2.operations.ProvisioningJob.run(ProvisioningJob.java:188)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

and the Eclipse from the default Equinox setup:

org.eclipse.equinox.p2.core.ProvisionException: No repository found at http://download.eclipse.org/technology/m2e/releases/.
	at org.eclipse.equinox.internal.p2.repository.helpers.AbstractRepositoryManager.fail(AbstractRepositoryManager.java:409)
	at org.eclipse.equinox.internal.p2.repository.helpers.AbstractRepositoryManager.loadRepository(AbstractRepositoryManager.java:674)
	at org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:110)
	at org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:105)
	at org.eclipse.equinox.p2.ui.LoadMetadataRepositoryJob.doLoad(LoadMetadataRepositoryJob.java:126)
	at org.eclipse.equinox.p2.ui.LoadMetadataRepositoryJob.runModal(LoadMetadataRepositoryJob.java:110)
	at org.eclipse.equinox.internal.p2.ui.sdk.PreloadingRepositoryHandler$1.runModal(PreloadingRepositoryHandler.java:84)
	at org.eclipse.equinox.p2.operations.ProvisioningJob.run(ProvisioningJob.java:187)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

I don't know how to proceed. Does it even make sense to attempt to change the dialog when it does not show in most situations?

merks · 2023-12-26T13:16:31Z

It seems to me one reason it doesn't show is because the failures might be accumulated instead of reported:

erik-brangs · 2023-12-27T14:52:34Z

Thank you for the hints. The dialog indeed only seems to be shown when accumulation of failures is disabled. Unfortunately, the new LocationNotFoundDialog is not reusable for the accumulation case because it assumes a ColocatedRepositoryTracker. The LoadMetadataRepositoryJob only provides a RepositoryTracker.

I don't see a way to handle the accumulated case without doing larger changes. Calling reportLoadFailure is not an option for the accumulated case because reportLoadFailure checks the reposNotFound in RepositoryTracker which have already been filled during the accumulation. That is, reportLoadFailure ignores repositories reported via addNotFound(URI) because the JavaDoc of addNotFound(URI) says that callers are assumed to have reported failures already. Handling the accumulated case would therefore mean changing the reporting from using IStatus to something else. I assume this would need new UI.

I have adjusted the original changes for the non-accumulated cases a bit. Is the PR suitable for merging in this state?

PS: I don't know why the build failed.

laeubi · 2023-12-27T15:02:57Z

@erik-brangs I don't completely understand why the accumulation is not usable here, is it because you miss the exception? Can you share an screenshot of the dialog to illustrate the problem maybe?

erik-brangs · 2023-12-27T15:48:38Z

The accumulation case implies - per the JavaDoc of the methods that its implementation uses - that the failures are already reported. You can therefore not use the reporting functionality from ColocatedRepositoryTracker or any other RepositoryTracker.

When the accumulation is done, the failure has not actually been reported to the user yet - but from the point of RepositoryTracker it has been reported because the failing locations were added to reposNotFound. Any call to reportLoadFailure on a RepositoryTracker will do nothing for repos that are contained in reposNotFound.

So you have to do the reporting for the accumulated case in LoadMetadataRepositoryJob. This is currently done via the IStatus as shown by Ed's screenshot from the accumulation.

The LocationNotFoundDialog uses methods like getMetadataRepositoryManager which are only available on ColocatedRepositoryTracker. The dialog could be rewritten to use ProvUI.getMetadataRepositoryManager(..) and ProvUI.getArtifactRepositoryManager(..).

However, the dialog only handles the failure for one site:

During the accumlation case, multiple failures could occur. How should those be handled? I don't think it would be a good idea to display the dialog for each site. So the accumulation case needs new UI.

laeubi · 2023-12-28T07:15:03Z

Thanks for the explanation and screenshot, that makes it now a bit clearer.

During the accumlation case, multiple failures could occur. How should those be handled? I don't think it would be a good idea to display the dialog for each site. So the accumulation case needs new UI.

I think the best would be to check at the place where the error dialog is shown if the accumulated failures are only caused by updatesites not found (is this possible?).

If that is the case, a dialog similar to yours can be shown that ask if they should be disabled or removed altogether would be enough, if it is just one single failed one then showing the existing dialog.

If it makes things easier we also can always assume they are colocated I'm not aware of any case where a user can enter individual items.

erik-brangs · 2023-12-29T12:35:00Z

I've tried to adjust the PR according to the feedback. I'm not sure why the ECA check fails. According to the Eclipse Foundation website, my ECA is current.

laeubi · 2023-12-29T14:26:45Z

@erik-brangs if you click on the failed check you should get additional information that (hopefully) help you to see whats wrong.

erik-brangs · 2023-12-29T14:41:34Z

I already did that and the error message made no sense to me. It said there was no ECA for my account which isn't true.

The ECA verification tool on https://api.eclipse.org/git/eca/status/gh/eclipse-equinox/p2/343 says there's no Eclipse Foundation account for my email which can't be true because it just logged in with my account at the Eclipse Foundation website.

laeubi · 2023-12-29T15:33:43Z

Did you try the troubleshooting?

A common error experienced by users is that the email associated with commits do not align with the email set in the Eclipse Foundation account. If you have an Eclipse Foundation account and a signed ECA-equivalent document, there are ways to validate the users and email addresses associated with a commit.

Below are 3 sample commands that will display the committer and author emails for a given commit or range. The first command shows the latest commit, while the second command will show results for a given commit, replacing <SHA> with the commit SHA. The last example will render a range of sequential commits. As is, it will show the last 5 commits starting from HEAD for the current branch. Both sides of the range can be replaced with commit SHAs to represent a different range.
git show -s --format='Commit: %h, Committer: %ce, Author: %ae
git show -s --format='Commit: %h, Committer: %ce, Author: %ae' <SHA>
git show -s --format='Commit: %h, Committer: %ce, Author: %ae' HEAD...HEAD~5 

Also make sure that your github user is set in the account details.

erik-brangs · 2023-12-29T15:42:05Z

There seems to be a general problem with the ECA checks: https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4112

erik-brangs · 2023-12-29T17:56:57Z

It seems the problem with the ECA check is fixed. Revalidation of the ECA succeeded.

mickaelistria · 2024-01-08T10:57:17Z

I find 4 buttons in a row is very heavy. Have you considered just adding a link to the "Install/Updates > Available Sites" preference page?

erik-brangs · 2024-01-08T11:47:18Z

In my installations, the "Available Sites" preference page normally has more sites than can fit on one page. In that case, I'd need use the filtering on the preference page. That means I have to remember which site failed. As I wrote in the original issue, one reason for the change was to relieve the user from remembering the failing site.

laeubi · 2024-01-10T07:33:26Z

These dialogs should not show up very often and the intention is for a user to quickly fix the issue "in place" without having to browse lists and perform additional steps so I think it is fine.

If one wants the Dialog can be improved later on, as currently there is effectively no way at all as to dismiss the dialog completely.

@merks do you think your concerns are addressed and this can be merged? Anything left now?

merks

If the host is bogus, it doesn't do much:

But if one specifies something bogus on an actual host, I finally see this dialog:

So that seems okay.

I have not reviewed the code in detail, I assume @laeubi will do that.

We should probably also rebase to ensure that we're not missing version increments.

laeubi · 2024-04-25T05:50:59Z

But if one specifies something bogus on an actual host, I finally see this dialog:

I would say: something that do not exits, but yes we can assume that at least at one point in time the URL was valid and the udatesite just vanished (or most likely was http in the past).

…ere update sites are not found (eclipse-equinox#207) Previously, if accumulation of load failures was enabled, all failures were put into a MultiStatus and displayed to the user. This has been changed to use more specific dialogs if possible. If all load failures were caused by repositories not being found, dialogs that allow removing or disabling the affected sites are used. If there is only a single load failure and it was caused by a repository not being found, the "Error Contacting Site" dialog is used. This commit extends the "Error Contacting Site" dialog with buttons for editing, removing or disabling a single site.

laeubi · 2024-04-25T05:53:40Z

...g.eclipse.equinox.p2.operations/src/org/eclipse/equinox/p2/operations/RepositoryTracker.java

It seems some reformating not related to the change have slipped in

That's just trimming of unnecessary whitespace, probably via save actions. I can remove it later if it's that important.

laeubi

Code looks good.

laeubi force-pushed the issue-207-additional-buttons-no-link-rewriting branch from 92657ac to b204735 Compare October 28, 2023 16:41

laeubi reviewed Oct 29, 2023

View reviewed changes

...org.eclipse.equinox.p2.ui/src/org/eclipse/equinox/internal/p2/ui/LocationNotFoundDialog.java Show resolved Hide resolved

laeubi reviewed Oct 29, 2023

View reviewed changes

...x.p2.tests.ui/src/org/eclipse/equinox/p2/tests/ui/operations/LocationNotFoundDialogTest.java Outdated Show resolved Hide resolved

erik-brangs force-pushed the issue-207-additional-buttons-no-link-rewriting branch from 07676b9 to d09184b Compare October 29, 2023 23:07

laeubi requested a review from merks December 26, 2023 12:27

erik-brangs force-pushed the issue-207-additional-buttons-no-link-rewriting branch 2 times, most recently from 0969b77 to 2eeca52 Compare December 27, 2023 14:30

laeubi mentioned this pull request Dec 28, 2023

Do not duplicate failed repository attempts #420

Merged

erik-brangs force-pushed the issue-207-additional-buttons-no-link-rewriting branch from 2eeca52 to a34968f Compare December 29, 2023 12:31

merks approved these changes Apr 24, 2024

View reviewed changes

erik-brangs added 2 commits April 25, 2024 07:51

Remove unnecessary whitespace from RepositoryTracker

506aa76

laeubi force-pushed the issue-207-additional-buttons-no-link-rewriting branch from a34968f to 8c2d65c Compare April 25, 2024 05:51

laeubi reviewed Apr 25, 2024

View reviewed changes

laeubi approved these changes Apr 25, 2024

View reviewed changes

laeubi merged commit 0e4707e into eclipse-equinox:master Apr 25, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 207 - Add additional buttons für deleting and removing update sites to the dialog "Error Contacting Site" #343

Issue 207 - Add additional buttons für deleting and removing update sites to the dialog "Error Contacting Site" #343

erik-brangs commented Oct 2, 2023

github-actions bot commented Oct 28, 2023 •

edited

Loading

laeubi commented Oct 28, 2023

merks commented Oct 28, 2023

laeubi commented Oct 28, 2023

laeubi commented Oct 29, 2023

laeubi commented Oct 30, 2023

merks commented Oct 30, 2023

erik-brangs commented Oct 30, 2023

laeubi commented Nov 3, 2023

merks commented Nov 3, 2023

laeubi commented Dec 9, 2023

erik-brangs commented Dec 10, 2023

erik-brangs commented Dec 20, 2023

merks commented Dec 26, 2023

erik-brangs commented Dec 27, 2023

laeubi commented Dec 27, 2023

erik-brangs commented Dec 27, 2023

laeubi commented Dec 28, 2023

erik-brangs commented Dec 29, 2023

laeubi commented Dec 29, 2023

erik-brangs commented Dec 29, 2023

laeubi commented Dec 29, 2023

erik-brangs commented Dec 29, 2023

erik-brangs commented Dec 29, 2023

mickaelistria commented Jan 8, 2024

erik-brangs commented Jan 8, 2024

laeubi commented Jan 10, 2024

merks left a comment

laeubi commented Apr 25, 2024

laeubi Apr 25, 2024 •

edited

Loading

erik-brangs Apr 25, 2024

laeubi left a comment

Issue 207 - Add additional buttons für deleting and removing update sites to the dialog "Error Contacting Site" #343

Issue 207 - Add additional buttons für deleting and removing update sites to the dialog "Error Contacting Site" #343

Conversation

erik-brangs commented Oct 2, 2023

github-actions bot commented Oct 28, 2023 • edited Loading

Test Results

laeubi commented Oct 28, 2023

merks commented Oct 28, 2023

laeubi commented Oct 28, 2023

laeubi commented Oct 29, 2023

laeubi commented Oct 30, 2023

merks commented Oct 30, 2023

erik-brangs commented Oct 30, 2023

laeubi commented Nov 3, 2023

merks commented Nov 3, 2023

laeubi commented Dec 9, 2023

erik-brangs commented Dec 10, 2023

erik-brangs commented Dec 20, 2023

merks commented Dec 26, 2023

erik-brangs commented Dec 27, 2023

laeubi commented Dec 27, 2023

erik-brangs commented Dec 27, 2023

laeubi commented Dec 28, 2023

erik-brangs commented Dec 29, 2023

laeubi commented Dec 29, 2023

erik-brangs commented Dec 29, 2023

laeubi commented Dec 29, 2023

erik-brangs commented Dec 29, 2023

erik-brangs commented Dec 29, 2023

mickaelistria commented Jan 8, 2024

erik-brangs commented Jan 8, 2024

laeubi commented Jan 10, 2024

merks left a comment

Choose a reason for hiding this comment

laeubi commented Apr 25, 2024

laeubi Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

erik-brangs Apr 25, 2024

Choose a reason for hiding this comment

laeubi left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 28, 2023 •

edited

Loading

laeubi Apr 25, 2024 •

edited

Loading