eclipse-equinox / p2

Eclipse Public License 2.0
14 stars 39 forks source link

Use RepositoryTracker for KnownRepositories to be consistent with Update #513

Closed laeubi closed 3 months ago

laeubi commented 3 months ago

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)

github-actions[bot] commented 3 months ago

Test Results

  375 files  +  125    375 suites  +125   43m 23s :stopwatch: + 13m 23s 1 893 tests ±    0  1 890 :white_check_mark: ±    0  3 :zzz: ±0  0 :x: ±0  6 679 runs  +2 228  6 670 :white_check_mark: +2 225  9 :zzz: +3  0 :x: ±0 

Results for commit 7c7b2e8b. ± Comparison against base commit 0f3af2c4.

:recycle: This comment has been updated with latest results.

merks commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

iloveeclipse commented 3 months ago

I wonder why API tooling didn't report any issue here? See https://github.com/eclipse-equinox/p2/issues/514.