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

Add an ArtifactManager to allows unified managing of artifacts #590

Closed
wants to merge 3 commits into from

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Jan 5, 2025

Caching can greatly improve performance, basically P2 has two concepts
of where caching can occur, the transport and artifacts that can
originate from different sources. Currently there is only a CacheManger
for handling the first case and nothing for the second, maybe even more
important case. Because of this there are different approaches out in
the wild e.g. PDE Bundle Pools, Oomph shared P2 pools, and Tycho is
using its own cache in the m2 repository, but all of them suffer from
the fact that there is no real convenient entry point so different
things needs to be enhanced an managed.

This now adds a new abstraction service the ArtifactManager that allows
to intercept the process of downloading a logical artifact from a given
location and possibly providing an equivalent one that is already
present locally. There is also a default implementation that simply
delegates to the transport directly as before.

This also includes a commit from

to add a component property type for the service name that is then used here.

@laeubi laeubi requested a review from merks January 5, 2025 08:25
Copy link

github-actions bot commented Jan 5, 2025

Test Results

  365 files   -  10    365 suites   - 10   40m 51s ⏱️ - 3m 19s
1 904 tests ±  0  1 896 ✅  -   5  3 💤 ±0  4 ❌ +4  1 🔥 +1 
6 584 runs   - 128  6 573 ✅  - 130  6 💤  - 3  4 ❌ +4  1 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit db39119. ± Comparison against base commit 3a74faa.

♻️ This comment has been updated with latest results.

@laeubi laeubi force-pushed the add_artifact_manger branch from bb6ee1c to f379008 Compare January 5, 2025 08:41
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.

Looks generally fine. I've not looked in great details.

@laeubi laeubi force-pushed the add_artifact_manger branch 2 times, most recently from ffadf04 to 02ff25f Compare January 5, 2025 12:40
@eclipse-equinox-bot
Copy link
Contributor

eclipse-equinox-bot commented Jan 5, 2025

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.equinox.p2.tests.ui/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From d9ec819da8fa4b57d3bb728d69287fe2bd24ba76 Mon Sep 17 00:00:00 2001
From: Eclipse Equinox Bot <[email protected]>
Date: Sun, 5 Jan 2025 13:00:51 +0000
Subject: [PATCH] Version bump(s) for 4.35.0 stream


diff --git a/bundles/org.eclipse.equinox.p2.tests.ui/META-INF/MANIFEST.MF b/bundles/org.eclipse.equinox.p2.tests.ui/META-INF/MANIFEST.MF
index f903a4215..d1abd427b 100644
--- a/bundles/org.eclipse.equinox.p2.tests.ui/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.equinox.p2.tests.ui/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.equinox.p2.tests.ui
-Bundle-Version: 1.4.400.qualifier
+Bundle-Version: 1.4.500.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Require-Bundle: org.eclipse.core.runtime;bundle-version="3.29.0",
-- 
2.47.1

Further information are available in Common Build Issues - Missing version increments.

@laeubi laeubi force-pushed the add_artifact_manger branch from ed7f94e to 94b8ed4 Compare January 5, 2025 12:58
*
* @since 2.13
*/
@Retention(CLASS)
@Target(TYPE)
@ComponentPropertyType
public @interface AgentServicename {
public @interface AgentServiceName {
Copy link
Member

Choose a reason for hiding this comment

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

I think the change is reasonable, I just wanted to mention that renamings that just change the case of some letters in a file name cause funny side-effects on Windows because the file-system is case-insensitive and then git gets confused.
But one just has to get along with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

As the class is actually new (I just renamed it in the PR for the sake of testing) it hopefully do not confuses windows to much :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the improved name. It looks like a typo before. 😕

@laeubi laeubi marked this pull request as draft January 5, 2025 14:59
laeubi added 3 commits January 5, 2025 16:21
Caching can greatly improve performance, basically P2 has two concepts
of where caching can occur, the transport and artifacts that can
originate from different sources. Currently there is only a CacheManger
for handling the first case and nothing for the second, maybe even more
important case. Because of this there are different approaches out in
the wild e.g. PDE Bundle Pools, Oomph shared P2 pools, and Tycho is
using its own cache in the m2 repository, but all of them suffer from
the fact that there is no real convenient entry point so different
things needs to be enhanced an managed.

This now adds a new abstraction service the ArtifactManager that allows
to intercept the process of downloading a logical artifact from a given
location and possibly providing an equivalent one that is already
present locally. There is also a default implementation that simply
delegates to the transport directly as before,
@laeubi laeubi force-pushed the add_artifact_manger branch from d1e98e8 to db39119 Compare January 5, 2025 15:22
@laeubi
Copy link
Member Author

laeubi commented Jan 5, 2025

After all the merges and refactoring I recreated the PR from scratch here:

@laeubi laeubi closed this Jan 5, 2025
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.

4 participants