Cycle now detected in SDK build #1124
Comments
|
See https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/PR-390/2/ for a full build showing the error. |
Yes, if you think this is wrong, it would be great to have a minimal example to investigate on this, while development I saw similar problems with the existing integration-tests when fragments involved, but I think the itest only cover one fragment and one host, so maybe we have here a case with multiple fragments but different hosts? Can the aggregator job be run locally as well? It seem requiring a lot of submodules and setup of PGP stuff... |
|
Just from the code, I think this might be because
and in the p2 inf additionally requires swt cocoa fragment so it depends on two fragments and thus pull in two hosts, but that is just a guess here. |
|
You can run the aggregator locally with a plain |
|
Alright, I think the issue really is that This references Bug 361901 but I can't find anything about e4 in there, so maybe the p2inf here is a leftover? |
|
Another, maybe simpler, example is visible at https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/97/console . It can be reproduced locally with |
|
Note that the error on https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/97/console is actually more surprising and unexpected and looks like a bug at 1st glance. I didn't (yet?) find a reason why org.eclipse.e4.ui.css.swt would depend on org.eclipse.ui.monitoring. |
|
So the error is
As mentioned, I don't get why the org.eclipse.e4.ui.css.swt bundle would reference org.eclipse.ui.monitoring.tests. I So -at least for this case- I believe the bug is more probably in Tycho. |
|
This error doesn't happen if the case is reduced to the only 2 involved modules. |
|
The tricky part is that if you just look at one bundle (or only a few) you will get a much more limited set of items (e.g. here you see everything is resolved to system or maven deps) while when you look at the reactor there is the situation that some are resolved to reactor projects witch can reference others as well. The tricky part is, that e.g. a fragment must depend on its host, but if you require the host (e.g. swt) this project can not be build before the host and all its fragments are build. For this, this project then depends on SWT and all its fragments what complicates the whole thing a lot ... |
This comment was marked as outdated.
This comment was marked as outdated.
|
I can see the the |
|
I have not analyzed that in detail, but my current guess is that the Slicer is even to eager here and we would need something that I have planned for supporting a nice dependency-tree, that is only collect the direct requirements of a project... as the transitives are accounted by maven already, and when we compute the target platform for the project. |
That would be a very good enhancement, but I don't think we need it here. I believe there is first a bug somewhere (in Tycho or Platform) as there really should be no reason for this dependency to be added, even with all the greediness we can think of. |
monitoring.tests pull in a lot of stuff, and is a fragment, so if it get into to greedy it transitively might pull in things that lead to a cycle and maven only complains about the shortest-path, so currently I think ui.monitoring is actually referenced somewhere (transitivly) and then everything explodes ... but still this is a bug of tycho then I think, it would just be good to find a reproducer, but actually if we can't find one I'll try to fix it by using the platform.ui job. Beside that... it would even make the code simpler and fast here :-) |
|
The problem seems to be with the additional attachment of fragment to projects requiring the host.
|
|
problem shows up in Xtext too |
|
4.25 SDK nightly is now also failed: https://ci.eclipse.org/releng/job/I-build-4.25/79/console |
|
I'm debugging for (IInstallableUnit unit : avaiableIUs.query(QueryUtil.ALL_UNITS, null)) {
if (isFragment(unit) && !projectUnits.contains(unit)) {
Set<IInstallableUnit> fragmentsResult = slicer
.computeDependencies(Collections.singleton(unit), collectionResult).toSet();
if (fragmentsResult.size() > 1) {
// at least one of the resolved items depend on the fragment
dependentFragments.add(unit);
}
}
}I fail at understanding what's the goal of this code, it doesn't seem to check whether the fragment is even related to dependency resolution. |
|
@mickaelistria see comment here #1128 (comment) fragments are important for build-order (most prominent example is SWT) as they can contribute additional classes to the host (as tested by |
Current dependency graph is wrong because of this. Fragments do depend on their host, not the other way round. It's how p2 works, it's how Equinox works. SWT is a strange case here and that's why is has the ExtensibleAPI flag. Only for this strange case, fragments may be treated as dependencies of their host and not the other way round. But it's 1 corner case, here the general case is broken. |
Yes and that's how it currently is implemented. The problem is that if a third party (test-fragment in this case) depends on other stuff (as in contrast a swt fragment do not depend on anything than SWT)... |
The fact that the code iterates over all existing reactor fragments and add them under certain conditions, independently of whether their host is involved and of whether ExtensibleAPI is set is way too greedy. In the case of eclipse.platform.ui, the host is not even part of the initial dependencies and yet the fragment is added! Basically, only fragments whose host is in the dependency tree and does declare extensibleAPI should (maybe) be added, all other ones should be ignored. |
Fragments of the host are removed, fragments of a dependency are added because that something that has to "happen before", but is something not good to be expressed in a maven-model. |
|
Nerveless I'm currently working on this and we have a test-case now for the "not-swt-case" so I think we will have a solution soon that satisfies both cases. |
It's not what happens for eclipse.platform.ui. org.eclipse.ui.monitoring is not a dependency of org.eclipse.e4.ui.css.swt. The code I pasted above doesn't seems to make a difference between whether the host is in the dependency set of not, and later rules to remove fragments do not apply. |
|
The above code computes the dependencies of one unit (the fragment) against all resolved project dependencies (the collectionResult) and then checks if at least more than one (the unit itself) is a result, and thus something satisfies what this fragment requires and thus is a perquisite. As mentioned before, this might be too much and as it includes transitive items as well, but omitting them is not enough as I already verified), but @pzi-dsa example can reproduce the issue with just two bundles and one fragment, and that's what I'm currently focus on :-) |
|
Any ETA when you think it can be fixed? |
|
I have now made some tests and at least for the P2-Solver the |
|
p2 entirely ignores Eclipse-ExtensibleAPI, p2 does not add fragments when adding a bundle. Fragments are usually added because they're required by a feature. |
The P2 Planner does it as a fragment has a requirement to its host and thus is part of "resolve all including requirements", see |
|
It's not the p2 planner that adds fragments for SWT. In the "official" repo, there are p2 metadata that make the SWT bundle optionally require the fragment and p2 just honors that requirement. If you want to verify that, you can run p2 director on an empty directory and install some Platform bundle: unless this bundle is SWT or use similar trick, the fragments (eg tests) for this bundle wouldn't be installed. |
The test do not use a P2 inf, it works with or without an p2.inf
I dont talk about install time, thats something different.
You can't really "compile" the code, but it calls all the code that is used to resolve and inject dependencies and I can see that it injects the swt-fragments into the jface bundle.
OSGI resolution is performed much later in the VerifyClassPath Mojo. |
Not so different. p2 director == p2 planner. p2 does not greedily adds fragments unless there is something that instructs it to do so.
With 2.7.3, I didn't see that: a |
The difference is that Tycho do not use the p2 director, but uses the Projector see |
The testcase only executes |
|
I think the test is correct and test what it's expected to test correctly. But I think your interpretation of the test and what it implies for p2 and related expectations regarding fragment additions is wrong. |
|
But you are right running it from the command and running it from the tests yields different results, that's undesirable :-( |
This is a unit test, it's meant to give input to the (OSGi) DependencyResolver, it's not meant to be used or run by CLI. |
Sure but the order is checked before the I have created #1131 to further investigate this. |
|
As the |
The eclipse.platform.releng.aggregator build is now failing with
I think it's a consequence of recent changes in the lifecycle.
The error seems actually correct. So we should first investigate whether this can be solved on Platform side.
The text was updated successfully, but these errors were encountered: