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

Use RepositoryTracker for KnownRepositories to be consistent with Update #513

Merged

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented May 12, 2024

Currently if the user choose 'Check for Updates' the handler uses the ProvisioningUI (that uses RepositoryTracker internally) to choose the initial items to offer. The UpdateChecker (aka automatically search for updates) on the other hand uses a (potentially) different set of items.

This now first fetches the RepositoryTracker service and if there use that to query for the repositories, if not there it uses only NON_SYSTEM repositories (what is the default in
RepositoryTracker#metadataRepositoryFlags)

@laeubi laeubi requested a review from merks May 12, 2024 05:34
Copy link

github-actions bot commented May 12, 2024

Test Results

  375 files  +  125    375 suites  +125   43m 23s ⏱️ + 13m 23s
1 893 tests ±    0  1 890 ✅ ±    0  3 💤 ±0  0 ❌ ±0 
6 679 runs  +2 228  6 670 ✅ +2 225  9 💤 +3  0 ❌ ±0 

Results for commit 7c7b2e8. ± Comparison against base commit 0f3af2c.

♻️ This comment has been updated with latest results.

@merks
Copy link
Contributor

merks commented May 12, 2024

This is another behavior change where we don't really know who or what relies on the old behavior. More concerning is that it's just not immediately clear where are all the places that the system repositories will still surface. Certainly this constant is not much used:

image

But this one is widely used, with the selected one being the one I mentioned before and the one that you are addressing in this PR:

image

So that makes me question, where else does this crop up? While exploring with the debugger, I see that when installing something new, the planner sees many more repositories than meet the eye:

image

All of this begs the question, what exactly is a system repository? Who is supposed to add such a thing, the system? Who is supposed to see them, the underlying frameworks but not the user? What if downstream application developers add system repositories for some specific purpose for impacting update and install and now we make them not visible/used simply because we decided to treat the discovered repository references as system repositories and then went into the framework to eliminate the system repositories from being actually used. Eventually, if we sweep broadly enough, the system repositories might as well not exist, circling us back to the question of what is a system repository and what for what is it used? It makes me uncomfortable that we are changing something without fully understanding it nor fully understanding the impact of our changes beyond our own use cases.

@merks
Copy link
Contributor

merks commented May 12, 2024

Note that I would have less of an aversion to making behavior changes where we don't really know the consequences on downstream applications if it were possible for those applications to restore the current behavior quickly and easily, e.g., via a system property. Unfortunately we don't really know the original design intent and I don't think anyone will explain it to us. As such, we're kind of in a bind so it's probably best we err on the side of caution such that if someone complains, there is an immediate solution...

@laeubi laeubi force-pushed the fix_updatechecker_using_wrong_flags branch from 7db1bb3 to c2a2cbc Compare May 14, 2024 04:19
Currently if the user choose 'Check for Updates' the handler uses the
ProvisioningUI (that uses RepositoryTracker internally) to choose the
initial items to offer. The UpdateChecker (aka automatically search for
updates) on the other hand uses a (potentially) different set of items.

This now adds a new method that allow to pass the repository flags to be
used when search for an update and delegate it on the callers side to
use the Provisioning UI.
@laeubi
Copy link
Member Author

laeubi commented May 14, 2024

First I now adjusted the code so the caller can control what is "wanted", for platform I strongly believe we want the changed behavior because of the following reasons:

This is another behavior change

This is not a behavior change per se, it was simply inconsistent in the past where one functionality "Check For Updates" used IRepositoryManager.REPOSITORIES_NON_SYSTEM and the other (automatic check for updates) used IRepositoryManager.REPOSITORIES_ALL, whatever was the inital intend here it should be consistent. As the UI is what the user sees and the other is just an automation I believe the UI is in the "leading position" here.

where we don't really know who or what relies on the old behavior.

Every change breaks someones workflow for sure... nevertheless who ever "relied" on that behavior has always relied on something that was unreliable / unpredictable because as shown in the previous PR depends on:

  1. the inital repositories added
  2. the number of Update checks the user performed
  3. the nesting deep of referenced repositories
  4. if automatic update checks where enabled or not and the actual frequency configured

depending on that a user can potentially see more or less updates, it could even happen that after one update the next time it gets even more. I believe that this is not any behavior one wants nor one should rely on.
If one really wants all referenced repositories can be added explicitly what will give a consistent user behavior.

More concerning is that it's just not immediately clear where are all the places that the system repositories will still surface. Certainly this constant is not much used

At least I'm not aware of any more places but that's not surprising to me as the intend from the code and from the rare documentation is that I found is that it should not surface to the user, but in most other cases should be used.

While exploring with the debugger, I see that when installing something new, the planner sees many more repositories than meet the eye

That's because the planner obviously is not "the user" and requires that information, but the important part is that at this stage the update UIs are already selected so this is not different than what happens if I install new items (and I select contact all sites to find required items for example).

All of this begs the question, what exactly is a system repository? Who is supposed to add such a thing, the system? Who is supposed to see them, the underlying frameworks but not the user?

I have found for example this one that describes them as being added but never shown to the user, in this case it is a composite repository that has showed up suddenly, the issue also describes that a repository even permanently can mark it self as "system" or you can mark them inside the repository manager.

Note that I would have less of an aversion to making behavior changes where we don't really know the consequences on downstream applications if it were possible for those applications to restore the current behavior quickly and easily, e.g., via a system property.

I now added p2.ui.sdk.scheduler.update.useProvisioningUI that defaults to true

@laeubi laeubi force-pushed the fix_updatechecker_using_wrong_flags branch from c2a2cbc to 7c7b2e8 Compare May 14, 2024 05:00
Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed and insightful analysis! Your comment about installing new things "at this stage the update UIs are already selected" is an interesting and relevant point.

@laeubi laeubi merged commit b46100f into eclipse-equinox:master May 14, 2024
11 checks passed
@iloveeclipse
Copy link
Member

I wonder why API tooling didn't report any issue here? See #514.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants