eclipse-oomph / oomph

Eclipse Public License 2.0
6 stars 11 forks source link

Behavior of variables is not correct, when used in a filter condition #76

Closed stegeto22 closed 8 months ago

stegeto22 commented 8 months ago

A VariablesTask, which is defined conditionally, is always executed, if the variable is used in a filter condition.

Example:

exampleSetup

In this example (org.eclipse.setup.zip) the my.feature1 and my.feature2 are enabled depending on the setting of my.version. When selecting the S1 stream, then no feature should be active.

ConfirmationPage

But as shown on the confirmation page, the my.feature1 was enabled. That's because it is used in the filter of the CompoundTask Feature1. The my.feature2 behaves correctly.

I assume, the issue is located in SetupTaskPerformer.getSetupTasks() in the lines 1734-1742. There all VariableTasks, which are used in a filter, are added unconditionally. Maybe this section could simply be removed?

merks commented 8 months ago

Of course it all works well when Feature1 is disabled:

image

That code cannot simply be removed. It was added when support for conditional tasks was added:

https://github.com/eclipse-oomph/oomph/commit/1db9130d4f07899319df833c8f80346c746c862b

The support for conditional tasks is super tricky because nothing is actually executed in a flow control style. Rather tasks are gathered, where the conditions affect was gathered and what is not gathered.

You can imagine having a filter expression on some compound C1 that sets some variable to a particular value and that variable is then being used in a filter on some compound C2 that conditionally sets the value of a variable that alters outcome of the filter on C1. E.g., here is a non-nonsensical usage where Feature1, when my.feature1=true, will set my.version=1 which implies that my.feature.1 should be set to false by V1, which then implies Feature1's condition is false:

image

You can see how this just becomes circular when there is no order of execution.

So using the value of a variable that is conditionally set elsewhere as a filter variable is just not workable because there isn't really any flow control.

Probably you have a bigger example where this is just a small demonstrator, but somehow you must find a different or better way.

stegeto22 commented 8 months ago

Ok, I understand.

There is already a cycle detection, when the variables are expanded (see SetupCoreUtil.reorder used in SetupTaskPerformer.reorderVariables). Maybe that could be extended to support the filters?

merks commented 8 months ago

This is super fragile code such that touching it is likely to have surprising consequences. Also, my time is super limited for adding addressing something complicated that I suspect cannot be made to work properly.

Is this something you want to invest your time in trying to fix in Oomph itself via a contribution, or might your time be better invested in finding a way not to rely on conditionally initialized variables being used as a filter for other conditional task?

stegeto22 commented 8 months ago

I'll find a way to don't rely on conditionally initialized variables. Thanks.

merks commented 8 months ago

@stegeto22

Sorry about that and thank you for your understanding. I could go on and on about how swamped I am, but that's my problem not yours...