Failing polly test is breaking pre-merge testing

13 views
Skip to first unread message

Christian Kühnel

unread,
Aug 6, 2020, 9:21:53 AM8/6/20
to Polly Development
Dear Polly development team,

I've seen failing polly tests break pre-merge testing a few times in the last weeks. It's been a few broken revisions with Polly :: ScopInfo/memcpy-raw-source.ll
Currently this one is failing: Polly :: Isl/Ast/alias_checks_with_empty_context.ll 

Some of the breakages last for more than 20 hours. So that means that any pre-merge test run in that time or using a base commit from that time window will also fail. This is then a false-positive for all pre-merge test users.

The failing tests can also be seen on buildbot.

1) Can you please fix the currently failing test?

2) How can we prevent long failing Polly tests from breaking pre-merge testing? Should we remove polly from pre-merge testing?

Best,
Christian

Michael Kruse

unread,
Aug 6, 2020, 9:32:42 PM8/6/20
to Christian Kühnel, Polly Development
Hi,

AFIAU it is the committer's responsibility to ensure that the buildbots do not break. When breaking, the buildbots send an email to the author. If you notice a break not being fixed, you are free to revert the offending commit(s).

I am going to look and fix the current issue.

Michael





--
You received this message because you are subscribed to the Google Groups "Polly Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to polly-dev+...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/polly-dev/d021d89c-b3e4-4476-b1d9-fe795f02cabbn%40googlegroups.com.


--
Tardyzentrismus verboten!

Christian Kühnel

unread,
Aug 7, 2020, 3:26:06 AM8/7/20
to re...@meinersbur.de, Polly Development
Hi Michael,

AFIAU it is the committer's responsibility to ensure that the buildbots do not break. When breaking, the buildbots send an email to the author. If you notice a break not being fixed, you are free to revert the offending commit(s).

This is also my understanding in general, but I wasn't sure if this was also an agreement for polly community. In the future I'll revert such commits. 

In the short term, we probably need to keep checking which commit broke compilation/tests on the master branch and keep fixing/reverting that. In the medium term I would like to find a solution where this does not require human work and where we have some branch that is ALWAYS passing a basic set of tests and builds. I don't really have a good way on how to get there...
 
I am going to look and fix the current issue.

Linux builds buildbot and pre-merge are green again. Thank you!

However another commit just broke the pre-merge tests on Windows. I'll revert that next :(


Best,
Christian

Michael Kruse

unread,
Aug 8, 2020, 1:01:09 AM8/8/20
to Christian Kühnel, Michael Kruse, Polly Development
The offending commit was already reverted by Mehdi once because it also broke the MLIR buildbots. The pre-merge checks did not trigger for that patch for some reason. I would not notice a Polly buildbot break myself until I would update my local checkout. I am also not as aggressive with reverting commits, most authors receiving complaints from the buildbots fix the issues themselves.

The pre-merge also checking Polly is, like pre-merge checks in general, useful to notify authors that their patch will break some builds, which is better than getting these notifications post-commit only.

Maybe the pre-merge check could be improved by only applying patches to known-good revisions (instead of the latest HEAD). Broken HEADs would just cause patches to be merged against slightly out-of-date revisions, instead of causing false alarms. Unless we guard patch landing itself by a pre-merge check bot, HEAD occasionally being broken is unavoidable.

Michael


--
You received this message because you are subscribed to the Google Groups "Polly Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to polly-dev+...@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages