#610 slicer requirements failure reproduction case

[vaclavHala]

No description provided.

[laeubi]

@vaclavHala can you please sign the ECA?

[laeubi]

@vaclavHala do you want to try to provide a fix as you already debugged this? Maybe we should simply resolve all items together even in slicer mode?

[vaclavHala]

I have signed the ECA.

As for the fix, I will try a few things though I have to say I'm not an experienced eclipse/tycho user so I do not know all the intentions behind the code. So It is possible I will "fix" it by breaking something else.

Maybe we should simply resolve all items together even in slicer mode?

That would solve my immediate problem but I think the underlying problem would remain - the way I understand slicer is it should not fail even if some requirements are completely missing (not just in different location as is my case) as opposed to planner. This is what Location wizard has to say about planner/slicer directly in eclipse By default, all required software is added to the target based on its environment settings. Turning this option off allows software to be added with missing requirements and multiple environments.

[laeubi]

not fail even if some requirements are completely missing

Correct so maybe we are calling the resolver in a wrong way here... just give it a try, there are a lot of test that should cover most use cases.

[github-actions]

Unit Test Results

148 files  148 suites   47m 30s ⏱️
246 tests 243 ✔️ 3 💤 0 ❌

Results for commit 29e62f8.

♻️ This comment has been updated with latest results.

[vaclavHala]

@laeubi please how do I write a test which verifies that some maven invocation actually fails with some expected error?
I'm trying to write these test cases

slicerDoesNotFailWhenDependenciesExistInDifferentTargetLocation
slicerDoesNotFailWhenDependenciesDoNotExistInAnyLocation
plannerDoesNotFailWhenDependenciesExistInDifferentTargetLocation
plannerFailsWhenDependenciesDoNotExistInAnyLocation

and for the last one I wanted to have verification like this

verifier.executeGoals(Arrays.asList("package"));
verifier.verifyTextInLog("Missing requirement: bundle2 1.0.0 requires 'osgi.bundle; bundle1 0.0.0' but it could not be found");

but the executeGoals fails if exit code is non zero and looking into the Verifier I can'f find any way to just run maven and not have any expectations about the result.

Obviously I can do this

try {
    verifier.executeGoals(Arrays.asList("package"));
} catch (VerificationException e) {
    // expected
}
verifier.verifyTextInLog( ...

but that is ugly and might catch some exception I'd actually want to make the test fail, like this one

throw new VerificationException( "Failed to execute Maven: " + e.getMessage(), e );

[laeubi]

but that is ugly

Yeah but I don't know of a better way here. With any other error your second verification will most likely fail.

[vaclavHala]

Ok so I looked into it a bit more and the best way I found was to not treat warnings from Slicer as failure.
Since both the Slicer and data it works on (i.e. Collection<IInstallableUnit> getRootIUs() and Collection<IRequirement> reqs = getRequirements(iu);) come from eclipse I treat their behavior as "right" so the only possible place to change something was in tychos interpretation of those.

Now behavior of the target resolution by tycho is consistent with what I see in the target editor of my eclipse when I open the four target files from the four test cases I added.

[laeubi]

Great, can you squash the both commits and make sure there are no unintentional changes?

[vaclavHala]

Sure I can do that.

It seems however that there are tests in other projects (I only tried running the tycho-it before pushing) which explicitly assert the current behavior

TargetDefinitionResolverIncludeModeTest.testUnsatisfiedInclusionWithSlicerFails 
TargetDefinitionResolverTest.testResolveDependenciesAcrossLocations

Furthermore in TargetDefinitionResolverTest there is this bit

        @Override
        public IncludeMode getIncludeMode() {
            // the tests in this class work with either
            return IncludeMode.SLICER;
        }

which seems to suggest the resolution should work the same way for planner and slicer, that makes me a bit confused

[laeubi]

I think it should never fail for WARNINGS (thats how PDE behaves)

[vaclavHala]

Ok, so should I try to modify the existing tests which expect the wrong behavior?

I think it may be a relatively safe change as all builds using tycho which previously worked should still work and some which failed may now start to work, but I'd definitely need a go ahead on this from you or other maintainers.

[laeubi]

Ok, so should I try to modify the existing tests which expect the wrong behavior?

Yes please if the assert a warning that is wrong. We should still print warnings to the lo if possible and if you can change the test to assert the warning in the log that would be perfect.

I think it may be a relatively safe change as all builds using tycho which previously worked should still work and some which failed may now start to work, but I'd definitely need a go ahead on this from you or other maintainers.

Just go ahead as you said it should be safe to do so.

[vaclavHala]

I have pushed the updated tests. There were two things which I think still don't behave as they should but would require more serious modification which could actually lead to breaking builds for some tycho users so I just left a TODO and commented-out assertion in the affected tests

[laeubi]

It seems there is a conflict can you resolve it and maybe rebase/squash your change?