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

Refresh a subset of bundles if they provide missing requirements #422

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Jan 3, 2024

When refreshing large sets of bundles the resolver sometimes take a choice where a requirement can't be bound even though it is there and resolvable.

This adds some additional code to detect this situation and then refresh a smaller set of bundles that provide these until a max retry is reached, all bundles are resolved, or all providers have been tried to refresh already.

Copy link

github-actions bot commented Jan 3, 2024

Test Results

    9 files  ±0      9 suites  ±0   34m 36s ⏱️ + 2m 43s
2 183 tests ±0  2 179 ✅ ±0   4 💤 ±0  0 ❌ ±0 
6 639 runs  ±0  6 628 ✅ ±0  11 💤 ±0  0 ❌ ±0 

Results for commit d3c8dc6. ± Comparison against base commit 6f0466c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@tjwatson tjwatson left a comment

Choose a reason for hiding this comment

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

The code is fine given what it is trying to do. But I question if we can have a testcase that exhibits the behavior so we can add a test for this code path.

@laeubi
Copy link
Member Author

laeubi commented Jan 3, 2024

The code is fine given what it is trying to do. But I question if we can have a testcase that exhibits the behavior so we can add a test for this code path.

I'm not yet sure in wich form such a test-case would have to be provided, can I somehow "export" a bundle-set (metadata-only) and use it as an input for a test with simple-configurator?

@tjwatson
Copy link
Contributor

tjwatson commented Feb 7, 2024

@laeubi you may not like this, but to move this forward I would like to have an option that defaults to the current behavior so we can enable this for the problematic scenarios. I would rather do that now so that the ones impacted by this issue can try out the solution without risking others. Once we get more confident in the approach then we can flip the default or get rid of the option.

@laeubi
Copy link
Member Author

laeubi commented Feb 7, 2024

@tjwatson do you like to suggest how this should be implemented? Maybe as simple as a system property to check?

@tjwatson
Copy link
Contributor

tjwatson commented Feb 7, 2024

@tjwatson do you like to suggest how this should be implemented? Maybe as simple as a system property to check?

Yes, but use BundleContext.getProperty instead of System.getProperty.

When refreshing large sets of bundles the resolver sometimes take a
choice where a requirement can't be bound even though it is there and
resolvable.

This adds some additional code to detect this situation and then refresh
a smaller set of bundles that provide these until a max retry is
reached, all bundles are resolved, or all providers have been tried to
refresh already.
@laeubi
Copy link
Member Author

laeubi commented Feb 8, 2024

@tjwatson I now added this inside a block that is disabled by default and can be enabled with equinox.simpleconfigurator.deeprefresh=true

@laeubi laeubi requested a review from tjwatson February 8, 2024 09:47
Copy link
Contributor

@tjwatson tjwatson left a comment

Choose a reason for hiding this comment

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

LGTM,

Thanks for putting the option to enable here. As developers we should encourage committers and contributors to test with this enabled so we can evaluate enabling this by default in the future.

@laeubi laeubi merged commit fd70696 into eclipse-equinox:master Feb 9, 2024
9 checks passed
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.

2 participants