search

eclipse-equinox / p2

Eclipse Public License 2.0
14 stars 39 forks source link

Add a way to skip reference repositories #505

Closed tivervac closed 4 months ago

tivervac commented 4 months ago

This goes hand in hand with the PR in PDE. If there's a way to achieve this without editing the public API in IPlanner, I would much prefer it.

Seeing how FOLLOW_ARTIFACT_REPOSITORY_REFERENCES was always hardcoded to true in ProvisioningContext, I feel like someone wanted to implement something similar at some point, but didn't get around to it.

For versioning, I just followed the quick fixes, I hope this is correct.

merks commented 4 months ago

I’ll look in detail tomorrow. I just wonder how p2 director already supports the -followReferences option (off by default) with the current implementation.

https://help.eclipse.org/latest/index.jsp?topic=/org.eclipse.platform.doc.isv/guide/p2_director.html

I also recall when oomph’s p2 director task didn’t follow references and I had change something to enable that. That’s a few years back…

laeubi commented 4 months ago

@tivervac the value is hardcoded in the constructor but code creating the provisioning context can still override that value by calling setProperty(FOLLOW_ARTIFACT_REPOSITORY_REFERENCES, "false") so I don't think we need a new constructor parameter but make the property public. @merks thats exactly how DirecorApplication works, but currently uses a copy of the constant FOLLOW_ARTIFACT_REPOSITORY_REFERENCES

Also note that there is another property FOLLOW_REPOSITORY_REFERENCES for metadata repositories what already is public.

merks commented 4 months ago

This is specifically where the director application configures that:

https://github.com/eclipse-equinox/p2/blob/1b21340cef3b1cdcc22ccf687835be9e925cc344/bundles/org.eclipse.equinox.p2.director.app/src/org/eclipse/equinox/internal/p2/director/app/DirectorApplication.java#L1063-L1064

So I don't think this PR is needed because one can always achieve the goal by configuring the context properties.

laeubi commented 4 months ago

So I don't think this PR is needed because one can always achieve the goal by configuring the context properties.

As mentioned the property is private, we should make it public instead of maintain just another copy at PDE soon. Beside that, it might be worth to have a method ProvisioningContext#setFollowReferences(boolean) that simply sets both properties to true/false to cover the common case and make this more obvious.

merks commented 4 months ago

Let's not pollute the API with convenience methods that then must interact properly with the property settings. In any case, that's not what this PR does.

Note that this is public:

But this is not:

That one could and probably should be made public and that would avoid the copy in the director:

image

But that's also not what this PR does. So we don't need this PR but a different PR to make the constant public seems helpful.

tivervac commented 4 months ago

@tivervac the value is hardcoded in the constructor but code creating the provisioning context can still override that value by calling setProperty(FOLLOW_ARTIFACT_REPOSITORY_REFERENCES, "false") so I don't think we need a new constructor parameter but make the property public. @merks thats exactly how DirecorApplication works, but currently uses a copy of the constant FOLLOW_ARTIFACT_REPOSITORY_REFERENCES

Also note that there is another property FOLLOW_REPOSITORY_REFERENCES for metadata repositories what already is public.

Right, the copy is what threw me off. I did a find references on FOLLOW_ARTIFACT_REPOSITORY_REFERENCES and didn't see it used in any set context. I'll make the constant public to make sure other people don't hit the same mistake. Once I've confirmed that setting the properties solves my issue, I'll close this PR. Thanks for the help both!

tivervac commented 4 months ago

Settings the property works for my usecase \o/

merks commented 4 months ago

@tivervac

Thanks for taking the time to contribute to such a complex project where it's all too easy to be thrown off by the endless details!

tivervac commented 4 months ago

Replaced by https://github.com/eclipse-equinox/p2/pull/506/