-
Notifications
You must be signed in to change notification settings - Fork 273
Visual Studio 2013 -> 2015 #2576
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
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.
This PR failed Diffblue compatibility checks (cbmc commit: 01a7cdf).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/78902247
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
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.
Looks good. A couple of other places to also consider cleaning up....
doc/cbmc-user-manual.md line 120, should probably drop '2013' (URL now points at 2017)
src/goto-programs/goto_program.h has some preprocessor #if #else #endif to handle Visual Studio <= 2013 - so might be nice to remove that at the same time? Actually, there's a number of places in the code base with macros to handle < 2013...
In line with what @chrisr-diffblue said: almost all matches of |
We have migrated all Windows builds to AWS CodeBuild now. I've removed the appveyor integration and added a commit here to remove the config as well. |
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.
This PR failed Diffblue compatibility checks (cbmc commit: 70443ea).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79106783
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
Is it possible to access the logs of those in some way? As is, this seems like a rather contributor-unfriendly step to me. |
Will squash and merge. |
I'm sure there's a way to set public ro permissions on codebuild... |
Not that I'm aware of. Or at least: not yet. You'd have to copy the build log to an S3 bucket. |
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.
There are couple of items that have been discussed but have not actually happened yet:
- Public access to codebuild logs.
- Further cleanup if genuinely moving to VS 2015.
- A statement of the reason for these changes.
Ok. I'll check with Subin Mathew. |
Furthermore #2495 strictly must be fixed and merged before this one, otherwise we are simply lying to ourselves (cf. |
70443ea
to
3193d20
Compare
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.
This PR failed Diffblue compatibility checks (cbmc commit: 3193d20).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79186322
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
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.
Passed Diffblue compatibility checks (cbmc commit: c4217b4).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79207404
c4217b4
to
a41356b
Compare
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.
Passed Diffblue compatibility checks (cbmc commit: a41356b).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79271014
a41356b
to
bedaa24
Compare
bedaa24
to
0764554
Compare
This is now documentation only. |
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.
This PR failed Diffblue compatibility checks (cbmc commit: bedaa24).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79439716
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
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.
Passed Diffblue compatibility checks (cbmc commit: 0764554).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79440188
I'd suggest to put this PR on hold until the continuous integration story has been sorted out. While I accept that it is "documentation only", it documents a state that is neither reflected in CI nor in the code base, both of which are geared towards VS 2013. Once CI is sorted out, I would suggest to add the changes that clean up the code base (cf. #2576 (comment)) and merge the combined work. |
Reviewer is on holiday. Contentious commits have been removed from this PR.
CodeBuild runs VS 2015: https://github.com/diffblue/cbmc/blob/develop/buildspec-windows.yml#L17 |
Reviewer is back from holiday. And still thinks that this is incomplete and should not be merged as-is. I accept that the Windows CI may now be using Visual Studio 2015, but its output remains behind closed doors. Much more importantly, though, the code base still has code that explicitly handles the VS 2013 case. Once that's removed this PR makes much more sense. |
Closing as #3894 has been merged, which is the same change. |
No description provided.