eclipse-pde / eclipse.pde

Eclipse Public License 2.0
25 stars 62 forks source link

org.eclipse.core.resources.IncrementalProjectBuilder.getRule(int, Map<String, String>) not properly implemented #535

Open laeubi opened 1 year ago

laeubi commented 1 year ago

I recently encountered this problem:

according to @iloveeclipse this is a regression from:

mickaelistria commented 1 year ago

I suspect the issue is that an empty MultiRule is not conflicting with the workspace but is contained. A first impression is that a non-conflicting rule shouldn't cause an issue; but that is a difficult area for quick changes. What could be possible and relatively safe is that in case the MultiRule is empty, we return null instead of an empty MultiRule.

iloveeclipse commented 1 year ago

the issue is that an empty MultiRule is not conflicting with the workspace but is contained

Exact this.

in case the MultiRule is empty, we return null instead of an empty MultiRule

Same here, quote from return javadoc on builder.getRule(): or <code>null</code> to indicate that no protection against resource modification during the build is needed.

laeubi commented 1 year ago

But if instead of empty null should be returned, and empty is invalid anyways, why not handle empty == null ?

iloveeclipse commented 1 year ago

The only thing I can't see right now: why PDE builder is active if there are no PDE projects in the workspace?

iloveeclipse commented 1 year ago

and empty is invalid anyways

it is not invalid per definition. But you can't push a rule that is empty in builder.

laeubi commented 1 year ago

it is not invalid per definition. But you can't push a rule that is empty in builder.

But somewhere there must be a call to getRule in the builder and there the caller should know that empty rule is not allowed?

mickaelistria commented 1 year ago

But somewhere there must be a call to getRule in the builder and there the caller should know that empty rule is not allowed?

There is no such concept of "empty" rule in ISchedulingRule, so the caller just deal with contains & isConflicting and shouldn't try to access the rule internal.

iloveeclipse commented 1 year ago

caller should know that empty rule is not allowed?

That's why we get an exception. But the code can't know if that empty rule is always conflicting, because it depends on other rule as well.

One possibility would be to catch that illegal state earlier in the BuildManager.basicBuild(int, IncrementalProjectBuilder, Map<String, String>, MultiStatus, IProgressMonitor), somewhere after the call to rule = builder.getRule(trigger, args); and report misbehaving builder to the log, so not the entire build is affected but only the current builder.

mickaelistria commented 1 year ago

But somewhere there must be a call to getRule in the builder and there the caller should know that empty rule is not allowed?

There is no such concept of "empty" rule in ISchedulingRule definition, so the caller just deals with contains & isConflicting and shouldn't try to access the rule internal. Concretely, an empty MultiRule is more or less an illegal value, it could have been prevented with some builder or other things to create MultiRule -or null- according to the input, but it's now too late to undo the current API.

laeubi commented 1 year ago

The only thing I can't see right now: why PDE builder is active if there are no PDE projects in the workspace?

I can "fix" the problem by creating one plugin project in the workspace, I think the builders are active because they are mentioned in the projects build spec, but the project does not has PDE nature, so one might be able to reproduce it by:

gireeshpunathil commented 1 year ago

apologies - button clicked by error. hope I reverted it.