Allow resolving mixed builds in eager resolve mode

[lppedd]

This reuse the same concept used in TychoReactorReader, more or less.

InstallableUnitGenerator takes for granted a JAR file has been built already for a project that is inside the same reactor.
This is not always the case.

[lppedd]

This is related to #2341.

[laeubi]

As already said, this is nothing Tycho should/would support.

[lppedd]

@laeubi there is nothing related to IDEs here. This simply allows resolving non-Tycho packagings without requiring them to have been built already.

[laeubi]

@laeubi there is nothing related to IDEs here. This simply allows resolving non-Tycho packagings without requiring them to have been built already.

If they are not build, Tycho can't use them because there is no generic way to know what packages they export, what bundle or version they will provide and so on.

[lppedd]

@laeubi obviously that is not the typical use-case.

IntelliJ IDEA injects a custom WorkspaceReader that returns a project's base directory in case that project doesn't have an artifact associated already. This demo PR shows that by letting Tycho work with a directory instead of a JAR file only, the entire flow works smoothly.

In any other context where there are no custom WorkspaceReaders injected, Tycho will work as before.

[laeubi]

A base directory is not enough. The general contract for a mixed build is that the project produces a jarfile and this jar file is a bundle, everything else is invalid and not supported.

So please provide an integration-test to demonstrate the issue with a plain mvn clean verify, and then we probably can change things showing that this use-case now works.

[github-actions]

Test Results

   549 files  +183     549 suites  +183   3h 39m 31s ⏱️ + 1h 6m 11s
   346 tests ±    0     340 ✔️ ±    0    6 💤 ±0  0 ❌ ±0 
1 038 runs  +346  1 019 ✔️ +339  19 💤 +7  0 ❌ ±0 

Results for commit 90498cf. ± Comparison against base commit a979cb0.

[lppedd]

@laeubi the problem here is that you see Tycho only from the build perspective. Not all tools that use Maven will use it to build.

Some tools only need to configure themselves based on how Maven generate its model.
In this specific case what happens is the Maven model will register a "mixed" jar-packaged project as a dependency of another tycho-packaged project, that's it. No build will ever occur.

This change allows the Mavel model configuration to finish without generating errors, that would have no relevance in the use-case where this change is used anyway.

[laeubi]

As said Tycho can't take the burden of other tools, if they need special treatment it must be handled there.

If no build ever occurs, the default Tycho mode will be enough and will never give any errors. Eager resolve is only for special cases where the Targetplatform is known in advance and one needs references to reactor projects via transitive dependencies and any other use-case it not supported, especially if it is not possible to write an integration test for this as this is completely internal behavior of Tycho and will change anytime in any way.

This change allows the Mavel model configuration to finish without generating errors

This seems very unlikely by the provided error you see, that says you depend on another module providing a bundle but Tycho can't know this before the bundle is build and therefore would be never satisfiable.

If you want something similar to what was done before (resolve as much dependencies as currently possible), this can be done by an ow extension (possibly reusing Tycho internal API), but this is not what Tycho resolving process does.

[laeubi]

If you want such behavior you should simply use the eclipse-plugin / eclipse-feature packaging for that project.

[laeubi]

This is an incompatible change, please the the documentation, consider is only the default for eager resolve is not configured.

[laeubi]

This is only valid in lazy mode, because in eager mode all dependencies are already resolved beforehand and can't be read from the pom (again).

[lppedd]

I'll address points here.

if they need special treatment it must be handled there

There is no way to handle mostly anything outside of Tycho as it is so "self contained" that everybody would have to replicate what Tycho does, which makes no sense.

this can be done by an ow extension (possibly reusing Tycho internal API)

Cannot be done because of the very changes I had to make here. See PomDependencies.ignore > PomDependencies.consider.
This is an implementation detail that would be impossible to change externally, and would conflict in the use of tycho-spi definitions such as TychoResolver in a custom extension.

If you want such behavior you should simply use the eclipse-plugin / eclipse-feature packaging for that project

Might be true, but a POM-specified dependency should be registered in the model without throwing errors if no build is occurring.

See https://github.com/openjdk/jmc/blob/91329f1b8d9896d5160e22262f06385bf43e960b/application/pom.xml#L151-L155 for the dependency declaration related to the error shown above.

This seems very unlikely by the provided error you see

I've tested the changes and everything is imported correctly. Obviously tested on JMC and other internal projects.

consider is only the default for eager resolve is not configured

That change is needed otherwise PomInstallableUnitStore will never return a useful query result.

if (considerPomDependencies == PomDependencies.ignore) {
    return EMPTY_RESULT;
}

Beware that this PR's changes are only meant to show how the problem is resolved.
I'm open to changing the way it's done.

[laeubi]

Cannot be done because of the very changes I had to make here. See PomDependencies.ignore > PomDependencies.consider.
This is an implementation detail that would be impossible to change externally, and would conflict in the use of tycho-spi definitions such as TychoResolver in a custom extension.

You don't need anything of that in an extension that reads the project dependencies and resolves against whatever source you want, the TychoGraphBuilder do it exactly that way. Also as said, you don't want a "Resolver" (that computes a satisfiable solution) you want a "Slicer" (that computes as much satisfying requirements as possible).

Might be true, but a POM-specified dependency should be registered in the model without throwing errors if no build is occurring.

This is already the case, but you are requesting to resolve an impossible problem because of that you get an error.

That change is needed otherwise PomInstallableUnitStore will never return a useful query result.

PomInstallableUnitStore is only valid and designed for lazy resolve, if you want eager resolve of pom dependencies use PomDependencies = consider or PomDependencies = wrap in your target platform.

Beware that this PR's changes are only meant to show how the problem is resolved.

The problem is not resolved, because it won't work if no Manifest is there, and if there is an manifest there is no reason for using a mixed build...

I'm open to changing the way it's done.

As said you should provide an integration-test to demonstrate the issue, if that's not possible then there is probably not a problem at Tycho at all...

[lppedd]

Also as said, you don't want a "Resolver"

I'm not entirely sure about this. The matter is I need the resolution process against p2 repositories too, I need all the stack. I don't need only inter-workspace resolution. A GraphBuilder only handles connections between reactor projects.

if you want eager resolve of pom dependencies use PomDependencies = consider or PomDependencies = wrap in your target platform

This actually helped, thanks. As this is done through IDE code, I had to add a configurable system property in this style

String value = getStringValue(configuration.getChild(POM_DEPENDENCIES), session, "tycho.target.pomDependencies", null);

Which is then set before execution

@Override
public void customizeUserProperties(
  final Project project,
  final MavenProject mavenProject,
  final Properties properties
) {
  properties.setProperty("tycho.target.eager", "true");
  properties.setProperty("tycho.target.pomDependencies", "consider");
}

The good side is I have reverted what you mentioned as a breaking change in TargetPlatformConfiguration#getPomDependencies.
Keeping the removal of if (!configuration.isRequireEagerResolve()) { ... } in P2ResolverImpl seems to be required still tho. The import fails if I reinstate the condition.

because it won't work if no Manifest is there

Well yes, there is a trade-off to be made, since it is impossible to handle all the cases. The code that checks for a MANIFEST or feature file under the project directly is very simple.

As said you should provide an integration-test

It's impossible to provide an integration test as the import process is handled through Maven Embedder in the IDE context.

[laeubi]

A GraphBuilder only handles connections between reactor projects.

It is quite trivial to enhance the concept to not only consider reactor projects, what the GraphBuilder actually do is that it computes a reactor IU set all projects provides and then resolves as much as possible, but the computation itself don't prevents considering other sources.

So you should somehow inject a custom component into your IDE build that do:

1. Implement a AbtractMavenLifecycleParticipant
2. In the setup phase check if the project is a Tycho one (--> TychoProjectManager)
3. Fetch the Targetplatform for the project (--> TargetPlatformService), you then has all items available via IMetadataRepository and IArtifactRepository
4. Compute the project IUs (-->org.eclipse.tycho.p2maven.InstallableUnitGenerator.getInstallableUnits(MavenProject, MavenSession, boolean)) then look at the dependencies of these units and find anything that probabbly matches in the targets IMetadataRepository
5. Now fetch any dependency you found form the IArtifactRepository and inject them somewhere in the maven model
6. don't use any eager or whatever special Tycho setting

Well yes, there is a trade-off to be made, since it is impossible to handle all the cases. The code that checks for a MANIFEST or feature file under the project directly is very simple.

As said in the comment, if the project has a manifest / feature it should use Tycho packaging type, as everything else will conflict with other plugins we have no control over how people configure these. So it is completely unclear where this MANIFEST / feature should come from if not from a build step in the pom.

I had to add a configurable system property in this style

this is not a system property but a user property, even though Tycho might evaluates System properties, you should not rely on this as this might change in future maven versions.

Nerveless that would be something one can add independently as a separate PR, we already have added support for user properties for other target related configuration option so I don't see a reason to not allow it here.

It's impossible to provide an integration test as the import process is handled through Maven Embedder in the IDE context.

Then it is impossible to support this in Tycho and the IDE uses non standard maven behavior, we only official support everything that can be covered by an integration test, adding "support" for things we can't test is simply not manageable as then we can't change the code without possibly breaking anyone (not covered) use cases. Tycho is a maven build plugin and not a general purpose library.

So you should really think about your use-case and what it actually includes and if this can be a usecase for a maven user, if not, you have to solve this elsewhere.

[laeubi]

@lppedd with this PR #2418 Tycho now can detect bnd-process and generates a preliminary manifest if eager resolution is requested (you still need to enable to consider pom dependencies just in case), probably you want to try that out if it helps with your issue, but depending on your exact configuration it might need some enhancements (e.g. if you use bnd:jar or similar).