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

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

Merged
merged 1 commit into from Jan 7, 2022

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Jan 2, 2022

Signed-off-by: Christoph Läubrich laeubi@laeubi-soft.de

@github-actions
Copy link

github-actions bot commented Jan 2, 2022

Unit Test Results

144 files  144 suites   49m 33s ⏱️
229 tests 226 ✔️ 3 💤 0

Results for commit 1609a1a.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

@mickaelistria this moves pom dependency metat-data computation out of the "maven-setup-phase" into the "target-resoloution-phase" and thus delays it until the final maven build starts.

This will (after some adjustments) fix the long outstanding issue that tycho is not able to consumer "pom-first" bundles together with tycho ones in the same reactor build.

@laeubi laeubi added this to the 2.6 milestone Jan 2, 2022
@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

All integration tests are green now, so from my side this is ready to go.
Even if there are some special cases where this not work, the maven-target location offers a more feature-rich opportunity to reference maven-dependencies in a tycho build.

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

We need to discuss #462 (comment) before merging anything. I'm afraid this could bring regressions.

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

We need to discuss #462 (comment) before merging anything. I'm afraid this could bring regressions.

Are you aware of any project using pomDependencies=consider in a larger context so I could check with these? I really don't want to support to much possible use case that are actually never used and not covered by test and given that there are alternatives that could be used to get back the old behavior I wont mind even a breaking change...

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

@HannesWell do you want to try this out with m2e build to gather some more experience beyond the simple integration test?

@HannesWell
Copy link
Member

@HannesWell do you want to try this out with m2e build to gather some more experience beyond the simple integration test?

Yes, I will test it with the m2e build.

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

Some minimal suggestions inline.
Also, please add documentation to TargetPlatformConfigurationMojo#pomDependencies to explicit how pom dependencies are now expected to participate or not in the resolution.

@laeubi
Copy link
Member Author

laeubi commented Jan 5, 2022

/rebase

@laeubi
Copy link
Member Author

laeubi commented Jan 6, 2022

@mickaelistria I rebased this now on your change regarding local file transfers.

@laeubi
Copy link
Member Author

laeubi commented Jan 6, 2022

only org.eclipse.tycho.test.product.ProductBuildTest is failing now @mickaelistria any hint what could be the cause?

@mickaelistria
Copy link
Contributor

See https://ci.eclipse.org/tycho/job/tycho-github/job/PR-470/28/#showFailuresLink ; ProductMixedVersionTest also fails with the same reason. The Maven artifacts are not visible into the "blackboard" repository used by p2 director.

@laeubi
Copy link
Member Author

laeubi commented Jan 6, 2022

okay found it, everything green now.

@mickaelistria
Copy link
Contributor

This looks good to merge soon, can you please squash the commits and add a commit message that describes the goal (delay resolution to allow for interacting with "lazily populated" Maven artifacts from reactor for better integration with eg Felix) and the most important part of the implementation (do not fail preliminary TP build -used for build order- is some requirements are missing most likely because of an artifact not yet available and let build continue further, and fail later if necessary + do not consider pom dependencies in extra p2-based build order computation) ?

@laeubi
Copy link
Member Author

laeubi commented Jan 6, 2022

I'll combine this into one commit and check the last TODO if I will do it now or later...

…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>
@laeubi
Copy link
Member Author

laeubi commented Jan 7, 2022

Everything is one commit now.

@mickaelistria mickaelistria merged commit 1609a1a into eclipse-tycho:master Jan 7, 2022
@mickaelistria
Copy link
Contributor

Thanks! Awesome change!
Please consider adding a note to the release notes about the capability to interact with other non-Tycho modules, that's a big big win to advertise ASAP!

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.

Delay Pom considered items to the final Target Platform calculation
3 participants