Add non-IU artifact repositories to the planner resolution #874

[ptziegler]

Given that units from non-IU locations are considered when resolving IUs, it may happen that units are contained in the resolution path, which are not contained by any repository. This may break previously functional target definitions.

To resolve this problem, an in-memory artifact and metadata repository is created, based on the metadata of the non-IU locations. Those repositories are then added to the provisioning context for further processing.

Amends dd321b5

[ptziegler]

The initial draft is looking good and the target definition is finally able to resolve again~

[laeubi]

I really like this approach, just a few comments and Jenkins reports some compiler warnings (even though they are not displayed correctly here).

[laeubi]

@jukzi as you are recently concerned about warnings, this is an example where one can see new warnings reported by Jenkins:

it is also shown in the code review (even though for some reason missing the message)

and the overview

@ptziegler sorry for the offtopic :-)

[vogella]

@laeubi could we fail the verification if new warnings are introduced?

[laeubi]

@laeubi could we fail the verification if new warnings are introduced?

Yes we can

• Enable the Quality Gate #964

[github-actions]

Test Results

     270 files  ±0       270 suites  ±0   52m 42s ⏱️ + 7m 21s
  3 329 tests +2    3 299 ✔️ +2  30 💤 ±0  0 ❌ ±0 
10 284 runs  +6  10 194 ✔️ +6  90 💤 ±0  0 ❌ ±0 

Results for commit bef392a. ± Comparison against base commit eb34cab.

♻️ This comment has been updated with latest results.

[ptziegler]

I've cleaned up the FileArtifactRepository and so far, everything should work. In case of artifical features, I create an in-memory jar only containing the feature.xml. Otherwise the location URL/path is used.

Note that I've renamed the repositories from FileRepository to VirtualRepository, given that they don't work on files anymore, so their name makes little sense anymore.

What I still need to figure out is how to properly inject the repositories into the provisioning context.
Reflection works, but it's not the correct solution...

I probably need proper setter methods... given that the indented extension points are not an option.

[laeubi]

What I still need to figure out is how to properly inject the repositories into the provisioning context.
Reflection works, but it's not the correct solution...

Extending the Provisiong context like you did seems a proper soloution to me...

[ptziegler]

What I still need to figure out is how to properly inject the repositories into the provisioning context.
Reflection works, but it's not the correct solution...

Extending the Provisiong context like you did seems a proper soloution to me...

Overwriting the method probably works, then, but certainly not the way it's currently done: :P

Field f = oldQueryable.getClass().getDeclaredField("repositories");
f.setAccessible(true);
artifactRepositories.addAll((List<IArtifactRepository>) f.get(oldQueryable));

[ptziegler]

What I still need to figure out is how to properly inject the repositories into the provisioning context.
Reflection works, but it's not the correct solution...

Extending the Provisiong context like you did seems a proper soloution to me...

Overwriting the method probably works, then, but certainly not the way it's currently done: :P

Field f = oldQueryable.getClass().getDeclaredField("repositories");
f.setAccessible(true);
artifactRepositories.addAll((List<IArtifactRepository>) f.get(oldQueryable));

Found the solution already: I can simply use a CompoundQueryable

[laeubi]

ProvisioningContext context = new ProvisioningContext(agent) {
	@Override
	public IQueryable<IArtifactRepository> getArtifactRepositories(IProgressMonitor monitor) {
	 	... enhance here ...
	}

	public IQueryable<IInstallableUnit> getMetadata(IProgressMonitor monitor) {
	 	... enhance here ...
	}
};

Found the solution already: I can simply use a CompoundQueryable

👍

[laeubi]

@ptziegler there is even org.eclipse.equinox.p2.query.QueryUtil.compoundQueryable(IQueryable<T>, IQueryable<T>)

[merks]

@ptziegler

I’m impressed by your resourcefulness! 🏆

[ptziegler]

The usage of reflection has also been removed. That should be it, from my side.

[ptziegler]

The new warning is unrelated to this change.

[INFO] --- tycho-p2-extras:4.0.5-SNAPSHOT:compare-version-with-baselines (compare-attached-artifacts-with-release) @ org.eclipse.pde.unittest.junit ---
[WARNING] Parameter 'comparator' is read-only, must not be used in configuration

warnings gone

[laeubi]

The new warning is unrelated to this change.

[INFO] --- tycho-p2-extras:4.0.5-SNAPSHOT:compare-version-with-baselines (compare-attached-artifacts-with-release) @ org.eclipse.pde.unittest.junit ---
[WARNING] Parameter 'comparator' is read-only, must not be used in configuration

Yes this is sadly due to new maven version adding new warnings :-
I think I will simply mark the parameter as readable to get rid of it:

• Remove readonly from parameter to prevent maven warning eclipse-tycho/tycho#3110

[laeubi]

Just one finding left, otherwise it looks good. 👍

[laeubi]

Looks good now, many thanks for your efforts in improving/fixing this.

[HannesWell]

Thank you @ptziegler for this PR.
This is good work and the functionality looks good.
Nevertheless I have a bunch of coding-style related remarks that should make some part simpler or more readable.

Because I was a bit short in time and didn't review this in the IDE, some remarks might not be applicable but are worth checking.

[ptziegler]

I hope I got all codestyle changes now...

[HannesWell]

Thank you for the updates.
I agree with all answers and just have one small suggestion to further simplify the copy.

With that being applied this is a perfectly fine change and also ready from my side.

[HannesWell]

Thank you.
This looks very good now 👍🏽

[laeubi]

I just wanted to note that the issue with two new maven warnings can be ignored if build/test is otherwise fine! I'm try to get rid of the warnings but its not that easy at least one of them should be fixed with this:

• [tycho-4.0.x] Check if artifact exits before attach it to not get warned by maven eclipse-tycho/tycho#3117

but will need probably 1 hour unless it show up.

[ptziegler]

Hm... I checked the generated feature.jar and now it's empty again...

There's clearly something about the BND tool that I'm doing wrong... argh

[HannesWell]

There's clearly something about the BND tool that I'm doing wrong... argh

Can't you just use the standard java Jar/ZipOutputstream or Jar/ZipFile instead of BND's Jar?

[ptziegler]

There's clearly something about the BND tool that I'm doing wrong... argh

Can't you just use the standard java Jar/ZipOutputstream or Jar/ZipFile instead of BND's Jar?

That's what I'm really tempted to do at this point. Debugging suggests the output-stream is never closed, but given that the BND class is 100% undocumented, I can't say whether that's by intent or what to do about it.

[HannesWell]

There's clearly something about the BND tool that I'm doing wrong... argh

Can't you just use the standard java Jar/ZipOutputstream or Jar/ZipFile instead of BND's Jar?

That's what I'm really tempted to do at this point. Debugging suggests the output-stream is never closed, but given that the BND class is 100% undocumented, I can't say whether that's by intent or what to do about it.

Please go for it. In general using standard Java instead of custom solutions should be preferred if the solutions are equivalent.

In general at the point when transfering the feature model to the destination OutputStrram what is the intended output? Should it just be the feature.xml or should it be in a jar?

[HannesWell]

In general at the point when transfering the feature model to the destination OutputStrram what is the intended output? Should it just be the feature.xml or should it be in a jar?

Looking at the code again, it is probably indeed a jar necessary.

[ptziegler]

Looking at the code again, it is probably indeed a jar necessary.

That was my impression as well.

Please go for it. In general using standard Java instead of custom solutions should be preferred if the solutions are equivalent.

And there we go...

[HannesWell]

Looks good now.
Thanks again.