Add local artifact repositories for resolution

[merks]

Fixes #881

[szarnekow]

Do we want to fail to add any shared bundle pool if a single Path.toUri throws an exception? Or should the try-catch be on a basis of a single pool?

[merks]

It's so much prettier with streams. 😱

Realistically 99.99% of the people (likely an huge under-estimate) have only one pool, and it is 0% likely that any of the paths cannot be converted to URIs because all hell would have broken loose long before p2/PDE ever reads them.

So technically yes, but realistically, not really.

[HannesWell]

I would say that actually it should never fail, otherwise the file is corrupted (e.g. wrong encoding) and it should be fixed.
So I would say it is ok to fail for all, but the catched exception should be logged instead so that a users notices the corruption: PDECore.log(Status.error("Failed to load shared bundle-pool locations",e)); or similar

[merks]

@laeubi was suggesting that maybe getSharedBundlePools should handle/log all the exceptions

eclipse-equinox/p2#377 (comment)

I tend to agree with him. What about you?

In any case, this commit was about using IRepositoryManager.REPOSITORIES_LOCAL and removing the condition. 😱

[HannesWell]

I tend to agree with him. What about you?

If all exceptions are handled there, I'm fine with that as well. But that should include the conversion to URIs.
If Oomph also needs a URI, it would make sense to handle all exceptions there, if there a different demands it is better handled by the callers (since they need some handling any ways).

In any case, this commit was about using IRepositoryManager.REPOSITORIES_LOCAL and removing the condition. 😱

Yes that's right. Nevertheless it would be good to find some kind of agreement on this quickly to reduce the trouble of multiple versions. 😅

[merks]

I have this change pending:

eclipse-equinox/p2#379

As such the only possible exception here is from the toURI call, but note that the called method has done an isDirectory test so I don't think we need to worry about to URI failing for an existing directory and needing to log it...

[merks]

And no, I don't want to convert to URIs in that method because I think it's important that these things are locations in the file system which is no longer clear/obvious when they are returned as URIs.

I think at some point here I will need to draw the line and call it done. Folks can continue do whatever they like. One may wish to revisit a number of things including things like this: 🙈

eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java

Lines 1502 to 1504 in e7d05ff

	} catch (CoreException e) {
	// if there is a problem, move on. Could log something here
	return;

The chance that toURI will fail for a Path for which isDirectory is true, where the value is the bundle pool used for all installations is pretty much zero, so talking about the value of logging that information is probably wasted breath. 😬

[HannesWell]

As such the only possible exception here is from the toURI call, but note that the called method has done an isDirectory test so I don't think we need to worry about to URI failing for an existing directory and needing to log it...

Makes sense.

Looking at the Java-doc of Path.toUri() I see only:

     * @throws  java.io.IOError
     *          if an I/O error occurs obtaining the absolute path, or where a
     *          file system is constructed to access the contents of a file as
     *          a file system, and the URI of the enclosing file system cannot be
     *          obtained
     *
     * @throws  SecurityException
     *          In the case of the default provider, and a security manager
     *          is installed, the {@link #toAbsolutePath toAbsolutePath} method
     *          throws a security exception.

Both are more fundamental problems that will probably apply to all paths and I don't expect any other than the default file-systems to be used.

[laeubi]

I think it's important that these things are locations in the file system which is no longer clear/obvious when they are returned as URIs

A Path can be a remote filesystem or even a virtual one... actually Path = URI + Browsing Capability + (optional) Watcher ;-)

[merks]

That's true. Even an archive has Paths to access it.

[github-actions]

Test Results

   264 files   -        6     264 suites   - 6   43m 9s ⏱️ - 18m 20s
3 328 tests +       1  3 297 ✔️ +       1  30 💤 ±  0  1 ❌ ±0 
7 557 runs   - 2 721  7 490 ✔️  - 2 697  66 💤  - 24  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 6957e78. ± Comparison against base commit e7d05ff.

♻️ This comment has been updated with latest results.