-
Notifications
You must be signed in to change notification settings - Fork 273
Add workaround for alpine regression test failure #1711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Would it be possible to take a look at #974? |
bbb47d8
to
2cc9b9f
Compare
CBMC doesn't handle assert on alpine-linux correctly. This works around this issue by using __CPROVER_assert directly in the tests that were previously failing. This appears to be fixed already on the develop branch
2cc9b9f
to
70882b7
Compare
@tautschnig That's probably the right thing to do for |
I'm afraid it will be a pain either way, unless you never want to merge into or from develop (but then really this project shouldn't be living here but instead be a proper fork). If you place fixes in goto-analyzer-develop that you don't want to port to develop then you're left with the problem of what (not) to cherry-pick. Welcome to the beautiful world of "we'll rebase later..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a case of fixed on develop
just that the alpine linux build has been turned off.
I think this is a reasonable approach, but if @tautschnig is concerned about clashing changes when G-A-D is merged into develop
, then you could similarly satisfy Travis by just dumping the Alpine build (or even better, cherry pick the PR(s) that modified the travis build).
I don't mean to stop you from merging this into G-A-D, but wouldn't want to see it in develop (as I believe #974 is the way to go - but of course I'm biased :-) ). |
@tautschnig I don't disagree. #974 is the right thing to do here, once that's merged something like this won't be needed on develop. Though it's a fairly benign change either way. I don't feel too strongly about doing this exact change, but I figured it'd be good to be able to get travis to not fail by default without completely disabling alpine builds or replicating #974 for GOA (which would be more work as the relevant files have diverged between develop and GOA). |
Just for a sense of completeness. I'm not planning on trying to merge g-a-d into develop. I've been sectioning bits of it up, PRing them and rebasing. The hope is that the distance between them will slowly reduce and eventually g-a-d will be able to rebase-track develop (or, maybe on one bright day in the future, not actually be needed any more!). |
That said ... I have been moving regression tests over wholesale so ... If @tautschnig doesn't want this in develop at all then I'm mildly reluctant to merge it somewhere I will forget and then attempt to merge it to develop. Other thoughts : what is actually blocking #974 ? Also, does anyone actually care about Alpine support? Last time I heard, the One Important User who wanted it originally didn't care any more as they'd moved their docker containers to Ubuntu. |
Not a code owner for #974 I'm afraid but @peterschrammel is and maybe he'll get this message and have time to take action... |
CBMC doesn't handle assert on alpine-linux correctly.
This works around this issue by using __CPROVER_assert
directly in the tests that were previously failing.
This appears to be fixed already on the develop branch