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

Delay Pom considered items to the final Target Platform calculation #462

Closed
laeubi opened this issue Dec 31, 2021 · 16 comments · Fixed by #470
Closed

Delay Pom considered items to the final Target Platform calculation #462

laeubi opened this issue Dec 31, 2021 · 16 comments · Fixed by #470
Assignees
Milestone

Comments

@laeubi
Copy link
Member

laeubi commented Dec 31, 2021

Currently pom-dependecies are calculate as part of the PreliminaryTargetPlatform and thus in the maven setup phase of Tycho.

As pom-dependecies are already part of the model the are not strictly required in the PreliminaryTargetPlatform and thus should be delayed until actually required (ReactorRepositoryManagerFacade.computeFinalTargetPlatform / ReactorRepositoryManagerFacade.getFinalTargetPlatform) to mitigate the following restrictions:

  • if only validate / clean goals are executed depending on the configuration heavy work will unnecessarily be performed
  • because they are invoked very early in the build before anything else is build it is not possible to reference a bundle of the current reactor build and thus makes it impossible to have a "mixed" reactor that contains plain maven projects together with tycho-plugin bundles.
@laeubi
Copy link
Member Author

laeubi commented Dec 31, 2021

FYI @HannesWell this will finally allow to have a "mixed" setup of plain maven plugins (e.g. felix-bundle-plugin bundle packaging) together with tycho eclipse-plugin packaging using pomDependecies=consider.

laeubi added a commit to laeubi/tycho that referenced this issue Dec 31, 2021
…atform

calculation

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@HannesWell
Copy link
Member

FYI @HannesWell this will finally allow to have a "mixed" setup of plain maven plugins (e.g. felix-bundle-plugin bundle packaging) together with tycho eclipse-plugin packaging using pomDependecies=consider.

That's great! So for eclipse-m2e/m2e-core#466 two builds would not be necessary any more? Thanks for looking into this.

* if only `validate` / `clean` goals are executed depending on the configuration heavy work will unnecessarily be performed

Not sure if this exactly related, but shouldn't the Tycho (Target-Platform) resolution be skipped for clean builds since #166?

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

Not sure if this exactly related, but shouldn't the Tycho (Target-Platform) resolution be skipped for clean builds since #166?

Yep but only if clean in the only phase I think.

Just consider the case that there is one project that takes very long (e.g. 30 seconds) to resolve its classpath.

If I now issue a clean install it takes 30 seconds before the build can start (given that the other plugins resolve in nearly zero time thanks to parallel execution).

With a delaying it to the actual build, while the slow porject resolves the others could be cleaned/build (given there are no inter-dependency relations between them).

@HannesWell
Copy link
Member

Not sure if this exactly related, but shouldn't the Tycho (Target-Platform) resolution be skipped for clean builds since #166?

Yep but only if clean in the only phase I think.

That's right. I also just wanted to mention it, I don't think it does not make this issue irrelevant.

Just consider the case that there is one project that takes very long (e.g. 30 seconds) to resolve its classpath.

If I now issue a clean install it takes 30 seconds before the build can start (given that the other plugins resolve in nearly zero time thanks to parallel execution).

With a delaying it to the actual build, while the slow porject resolves the others could be cleaned/build (given there are no inter-dependency relations between them).

Sounds promising. Do I understand you right, that the classpath resolution will be trigged for each project in a separate (java concurrent Executor?) task in the beginning of the build and when a particular project is next to be build Tycho waits for the cp-resolution task of that project to complete. And if the previous projects took long enough to build that task will already be completed. So if one is lucky and the first projects to build have a classpath to resolve quickly and take long enough to build, the additional total runtime of the cp-resolution can go to zero?

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

It then uses the usual maven -T setting an executes mojos in parallel if possible. Currently there is a "pre maven build" phase where the class-path is computed, we already parallelize this if requested but this still is bound to the longest project resolving time.

laeubi added a commit to laeubi/tycho that referenced this issue Jan 2, 2022
…cord those

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Jan 2, 2022
…cord those

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Jan 2, 2022
…Platform

calculation

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@mickaelistria
Copy link
Contributor

I'm not sure what you mean by "delay", do you mean they're ignored? If I'm not mistaken, the preliminary target platform is used in order to compute the build order; and the pomDependencies can affect the build order, so I don't get how the build order can remain correct if pom dependencies are ignored.

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

I'm not sure what you mean by "delay", do you mean they're ignored?

Moving it out of the phase where we compute build order and dely it until a later stage in the regular maven build.

and the pomDependencies can affect the build order,

Can you give any example? As pom dependencies are already there at the pom level there is no need for tycho to take those into account at this stage.

@mickaelistria
Copy link
Contributor

For example, bundles A and B are in reactor, bundle Z is a Maven dependency defined in pom.xml. A requires-bundle on Z, Z requires-bundle B and reexports. So packages from B must be visible to A, B must be built before A. If pomDependencies are ignored while computing build order, Tycho will lead to incorrect results in some cases, that would be a regression.

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

Could you please provide a runnable example (or better integration test) for this? Is this a real use-case or just some kind of academic example?

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

I have added an integration test to the PR now that shows a mixed reactor build, the output is as follows:

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Build Order:
[INFO] 
[INFO] mixed.reactor.parent                                               [pom]
[INFO] felix.bundle                                                    [bundle]
[INFO] tycho.bundle                                            [eclipse-plugin]
[INFO] 

<< snip >>

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for mixed.reactor.parent 1.0.0-SNAPSHOT:
[INFO] 
[INFO] mixed.reactor.parent ............................... SUCCESS [  0.065 s]
[INFO] felix.bundle ....................................... SUCCESS [  1.126 s]
[INFO] tycho.bundle ....................................... SUCCESS [  1.019 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

as one can see the felix.bundle is of packaging type bundle while tycho.bundle of packaging-type eclipse-plugin while tycho.bundle consumes a package from felix.bundle whose manifest is auto-generated during the build.

@HannesWell
Copy link
Member

For example, bundles A and B are in reactor, bundle Z is a Maven dependency defined in pom.xml. A requires-bundle on Z, Z requires-bundle B and reexports. So packages from B must be visible to A, B must be built before A. If pomDependencies are ignored while computing build order, Tycho will lead to incorrect results in some cases, that would be a regression.

Is this case even possible? Because Z is a Maven project (that has a OSGi compliant MANIFEST.MF generated in its final jar) it only uses the Maven mechanism to resolve its dependencies and therefore cannot know B, unless B is also a 'pure' Maven project.

I first also had concerns about cases like that, but came to the conclusion that Maven projects cannot depend on Eclipse-Plugin projects because they only have the Maven dependency mechanism, while Eclipse projects have Eclipse (possible extended by Maven) dependencies. So Maven projects can only depend on other Maven-projects while Eclipse-plugin projects can depend on both. In the end Maven projects have to respectively will be (if the pom-dependencies of a Eclipse plug-in are also considered in the Reactor order) always build first.

@mickaelistria
Copy link
Contributor

Is this case even possible? Because Z is a Maven project (that has a OSGi compliant MANIFEST.MF generated in its final jar) it only uses the Maven mechanism to resolve its dependencies and therefore cannot know B, unless B is also a 'pure' Maven project.

It is actually possible, assuming a variation (old version or other provider) of B pre-exists and is used when Z is built. Or a similar example can be considered with Import-Package, with another provider for the packages.

Is this a real use-case or just some kind of academic example?

It's more an academic example and the goal is more to discuss the change fully before deploying it in order to anticipate potential breakage it can cause. But I think the value of the change is very high and it can be acceptable to break some cases if there is a consensus they're fine to be broken.
I'll try to think about other potential issues in the next days.
In the meantime, we should maybe as part of this commit add a note to pomDependencies that "pom dependencies do not participate to extra build order computation" or something like that to clarify things.

@laeubi
Copy link
Member Author

laeubi commented Jan 3, 2022

Is this case even possible? Because Z is a Maven project (that has a OSGi compliant MANIFEST.MF generated in its final jar) it only uses the Maven mechanism to resolve its dependencies and therefore cannot know B, unless B is also a 'pure' Maven project.

It is possible as tycho computes the dependencies in an "dynamic" way, I can think of a simpler example.
We have A that depends on P(=pomdepdency) and P imports a package with range (1...3] then it might be possible that R(=reactor project) exports the package with a version 2 that is never than when P was build or depends on.

On the other hand P can only have a GAV dependency that matches exactly one version. From Maven POV the Tycho way might seem to "threaten the stability of the build", but that's how it works at the moment.

It's more an academic example

I think we should support people as much as possible, my experience with pomDependecies was that it was rather limited and hard to guess if something worked or not and finally has always chosen other ways. Thats why I wonder if there are some projects actually using this feature beyond very simple examples (e.g. injecting some additional dependencies that are not available as update-sites).

note to pomDependencies that "pom dependencies do not participate to extra build order computation"

I think we should even narrow this don more: pomDependencies should only resolve to pom declared dependencies as well, as described by @HannesWell this would make things much more clear and consistent. If one still needs such a case as described above there are some ways to accomplish this:

  1. one could simply use a maven target location that works exactly as pomDependencies today but much more flexible
  2. The consumer of the pomDependency could simply add another pomDependency to a reactor project (or any other dependency) for it to be considered additionally.
  3. We might enhance the current approach so support this better, but an integration test showing the problem should be provided and added to tycho.

@laeubi
Copy link
Member Author

laeubi commented Jan 3, 2022

I'll try to think about other potential issues in the next days.

What do you think about we start the 2.6.0 release (as people already have asked for) and then we have more time to find bugs/clean up things for 2.7.x...

@mickaelistria
Copy link
Contributor

Fixing bugs should happen before a releaser. Given the amount of recent and pending changes, the code is IMO too "fresh" to be released. If you want to complete the work about ignoring pomDependencies in build order resolution, then we should allow at least 1 extra week of testing to consumers after completion to verify it fixes the issues as expected and doesn't cause unexpected regressions.

@laeubi
Copy link
Member Author

laeubi commented Jan 3, 2022

I'm also fine with including it in 2.6.0 that's just an idea if we think we should take more time to investigate for this feature.
Technically we can also branch a 2.5.x from a previous commit (e.g. before my recent changes).

laeubi added a commit to laeubi/tycho that referenced this issue Jan 7, 2022
…Platform

- if pom dependencies are considered allow for a partial resolution of
the preliminary target platform
- don't use pom considered dependencies in computation of build-order
anymore
- pom considered dependencies are now part of the final target platform
calculation

These changes allow for other reactor projects to contribute to the OSGi
classpath results including for example bnd/felix bundle plugin.


Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
mickaelistria pushed a commit that referenced this issue Jan 7, 2022
- if pom dependencies are considered allow for a partial resolution of
the preliminary target platform
- don't use pom considered dependencies in computation of build-order
anymore
- pom considered dependencies are now part of the final target platform
calculation

These changes allow for other reactor projects to contribute to the OSGi
classpath results including for example bnd/felix bundle plugin.


Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi laeubi added this to the 2.7 milestone Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants