Skip to content

Move call_stack_history to own file #5384

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

Merged
merged 2 commits into from
Jun 23, 2020

Conversation

martin-cs
Copy link
Collaborator

A pure refactoring commit; does not alter any of the functionality, just divides things up a bit better.

martin added 2 commits June 18, 2020 14:55
I am about to add a few more kinds of history and having their own
files, like domains, probably makes more sense.
@martin-cs
Copy link
Collaborator Author

Please can I have some help here? There are CI failures but I can't actually access the details because my AWS account isn't authorised to view them. Is there any way that PRs can have open CI results? It is an unnecessary road-block to "external" people contributing to the project.

@thk123
Copy link
Contributor

thk123 commented Jun 18, 2020

@martin-cs we have a fix for external contributors accessing AWS logs coming Soon(tm) in this case the failure seems transient (404'ing on downloading win-flex-bison), I've restarted it, but failure on that job won't block merging PRs.

@martin-cs
Copy link
Collaborator Author

Thanks for the swift and helpful response.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving classes code into a separate file is always welcome

@thk123
Copy link
Contributor

thk123 commented Jun 18, 2020

Suggested improvement for clearer PR title:

Move call_stack_history to own file

Since initial title made me think this was to do with git history(!) and refactor sounds a lot more risky than move.

@martin-cs martin-cs changed the title Refactor/split up histories Move call_stack_history to own file Jun 18, 2020
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #5384 into develop will not change coverage.
The diff coverage is 74.64%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5384   +/-   ##
========================================
  Coverage    68.18%   68.18%           
========================================
  Files         1173     1175    +2     
  Lines        97494    97494           
========================================
  Hits         66478    66478           
  Misses       31016    31016           
Flag Coverage Δ
#cproversmt2 42.49% <ø> (+0.04%) ⬆️
#regression 65.36% <74.64%> (ø)
#unit 32.23% <ø> (+0.02%) ⬆️
Impacted Files Coverage Δ
src/analyses/ai_history.cpp 0.00% <ø> (-57.15%) ⬇️
src/analyses/ai_history.h 75.86% <ø> (-5.39%) ⬇️
src/goto-analyzer/goto_analyzer_parse_options.cpp 67.32% <ø> (ø)
src/analyses/call_stack_history.cpp 69.23% <69.23%> (ø)
src/analyses/call_stack_history.h 89.47% <89.47%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d02492...2f4108d. Read the comment docs.

@thk123
Copy link
Contributor

thk123 commented Jun 19, 2020

Dunno why the job isn't posting back (@peterschrammel any ideas?) But the retried build succeeded - so this is good to be merged with code owner approval. (for those with access, the successful build can be seen here: https://us-east-1.console.aws.amazon.com/codesuite/codebuild/767717039613/projects/cbmc-windows-cmake/build/cbmc-windows-cmake%3A9a049d78-b91a-4a51-80a7-cd21ac99efd0/?region=us-east-1)

@martin-cs
Copy link
Collaborator Author

@thk123 Thanks for chasing this.

@martin-cs
Copy link
Collaborator Author

@chrisr-diffblue @danpoe @hannes-steffenhagen-diffblue @peterschrammel @smowton
Sorry to pester but please can you have a look at this? As per @thk123 's comments, all we are waiting for is an owner review and it is literally just a code-reorganisation, there is no change to the functionality.

@peterschrammel
Copy link
Member

I've restarted the cbmc-windows-cmake job which seemed to have a network issue.

@martin-cs
Copy link
Collaborator Author

Brilliant! Thank you @peterschrammel

@martin-cs
Copy link
Collaborator Author

@peterschrammel Sorry to pester but the status hasn't changed here despite the restart and my AWS account doesn't have access rights to see why it has failed.

@hannes-steffenhagen-diffblue
Copy link
Contributor

Note the windows build “passes” now (not shown as such in the list because restarting the builds does not actually update them here on github for AWS for some reason).

“Passes” in quotes because the build actually appears to be broken for reasons unrelated to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes OK

@NlightNFotis
Copy link
Contributor

This is the last build log:

[Container] 2020/06/18 15:26:48 Running command choco install -y --no-progress winflexbison3 ninja
43 | Chocolatey v0.10.15
44 | Installing the following packages:
45 | winflexbison3;ninja
46 | By installing you accept licenses for the packages.
47 |  
48 | winflexbison3 v2.5.18.20190508 [Approved]
49 | winflexbison3 package files install completed. Performing other installation steps.
50 | WARNING: Url has SSL/TLS available, switching to HTTPS for download
51 | Downloading winflexbison3
52 | from 'https://sourceforge.net/projects/winflexbison/files/old_versions/win_flex_bison-2.5.18.zip'
53 | ERROR: The remote file either doesn't exist, is unauthorized, or is forbidden for url 'https://sourceforge.net/projects/winflexbison/files/old_versions/win_flex_bison-2.5.18.zip'. Exception calling "GetResponse" with "0" argument(s): "Unable to connect to the remote server"
54 | This package is likely not broken for licensed users - see https://chocolatey.org/docs/features-private-cdn.
55 | Environment Vars (like PATH) have changed. Close/reopen your shell to
56 | see the changes (or in powershell/cmd.exe just type `refreshenv`).
57 | The install of winflexbison3 was NOT successful.
58 | Error while running 'C:\ProgramData\chocolatey\lib\winflexbison3\tools\chocolateyInstall.ps1'.
59 | See log for details.
60 |  
61 | ninja v1.10.0 [Approved]
62 | ninja package files install completed. Performing other installation steps.
63 | Extracting C:\ProgramData\chocolatey\lib\ninja\tools\ninja-win_x32.zip to C:\ProgramData\chocolatey\lib\ninja\tools...
64 | C:\ProgramData\chocolatey\lib\ninja\tools
65 | Installed to: 'C:\ProgramData\chocolatey\lib\ninja\tools'
66 | ShimGen has successfully created a shim for ninja.exe
67 | The install of ninja was successful.
68 | Software installed to 'C:\ProgramData\chocolatey\lib\ninja\tools'
69 |  
70 | Chocolatey installed 1/2 packages. 1 packages failed.
71 | See the log for details (C:\ProgramData\chocolatey\logs\chocolatey.log).
72 |  
73 | Failures
74 | - winflexbison3 (exited 404) - Error while running 'C:\ProgramData\chocolatey\lib\winflexbison3\tools\chocolateyInstall.ps1'.
75 | See log for details.
76 |  
77 | [Container] 2020/06/18 15:27:33 Command did not exit successfully choco install -y --no-progress winflexbison3 ninja exit status 404
78 | [Container] 2020/06/18 15:27:33 Phase complete: INSTALL State: FAILED
79 | [Container] 2020/06/18 15:27:33 Phase context status code: COMMAND_EXECUTION_ERROR Message: Error while executing command: choco install -y --no-progress winflexbison3 ninja. Reason: exit status 404
80

It seems like it fails to download winflexbison3. This has happened to me before and in my experience in has mostly been network errors. Locally I've hit the url that returns a 404 and for me it returns a 301 moved - if I allow curl to follow redirects it downloads it without an issue - so I'm assuming the file is present in the host, it's just something that fails for another reason.

Worth noting that if I restart the build it passes, but it doesn't update the state here in github. Could you maybe just perform an action that changes the commit hash and push it again to force it to restart builds and hope for the best?

Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me as well.

@martin-cs
Copy link
Collaborator Author

Right, the windows build issues are beyond my access permissions (and expertise) to investigate or fix. Both @thk123 and @hannes-steffenhagen-diffblue say it works so I am just going to merge it. I can't see how it could break things (famous last words obviously).

@martin-cs martin-cs merged commit 823782c into diffblue:develop Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants