Skip to content

Fix compilation of the STP master branch -- it now depends on ABC and uses submodules#1750

Merged
MartinNowack merged 5 commits intoklee:masterfrom
ccadar:stp-abc
Oct 18, 2024
Merged

Fix compilation of the STP master branch -- it now depends on ABC and uses submodules#1750
MartinNowack merged 5 commits intoklee:masterfrom
ccadar:stp-abc

Conversation

@ccadar
Copy link
Contributor

@ccadar ccadar commented Oct 14, 2024 •
edited
Loading

Summary:

Fix compilation of the STP master branch -- it now depends on ABC and uses submodules: stp/stp#496.

Checklist:

  • The PR addresses a single issue. If it can be divided into multiple independent PRs, please do so.
  • The PR is divided into a logical sequence of commits OR a single commit is sufficient.
  • There are no unnecessary commits (e.g. commits fixing issues in a previous commit in the same PR).
  • Each commit has a meaningful message documenting what it does.
  • All messages added to the codebase, all comments, as well as commit messages are spellchecked.
  • The code is commented OR not applicable/necessary.
  • The patch is formatted via clang-format OR not applicable (if explicitly overridden leave unchecked and explain).
  • There are test cases for the code you added or modified OR no such test cases are required.

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 14, 2024
View reviewed changes
@codecov
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.14%. Comparing base (7cc669b) to head (9be184d).
Report is 24 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1750      +/-   ##
==========================================
+ Coverage   68.98%   70.14%   +1.16%     
==========================================
  Files         162      162              
  Lines       19446    19443       -3     
  Branches     4643     4638       -5     
==========================================
+ Hits        13414    13638     +224     
+ Misses       3999     3760     -239     
- Partials     2033     2045      +12

see 9 files with indirect coverage changes

@ccadar ccadar force-pushed the stp-abc branch 4 times, most recently from 861deab to 0bb9fe5 Compare October 17, 2024 12:38
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 17, 2024
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 17, 2024
View reviewed changes
@ccadar ccadar changed the title Fix compilation of the STP master branch -- it now uses submodules. Fix compilation of the STP master branch -- it now depends on ABC and uses submodules. Oct 17, 2024
@ccadar ccadar changed the title Fix compilation of the STP master branch -- it now depends on ABC and uses submodules. Fix compilation of the STP master branch -- it now depends on ABC and uses submodules Oct 17, 2024
@ccadar ccadar mentioned this pull request Oct 17, 2024
Merged
8 tasks
@MartinNowack
Copy link
Contributor

MartinNowack commented Oct 17, 2024

@ccadar I'm in general happy with this. But, this will break again for a newer STP releases. Could you add an additional version check for stp > 2.3.4? So we don't need to touch it twice.

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 18, 2024
View reviewed changes
@ccadar
When handling the ABC dependency of STP, compare the version with 2.3…
94f9189 
….4 instead of master, which will also handle future releases
@ccadar
Copy link
Contributor Author

ccadar commented Oct 18, 2024

@MartinNowack good suggestion. Not straightforward to compare versions in bash, but I think I found a good solution to it -- we now also have version comparison functions in common-functions

MartinNowack
MartinNowack approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@MartinNowack MartinNowack left a comment

Choose a reason for hiding this comment

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

@ccadar Fantastic! Thanks a lot for your hard work.

I'm curious about any potential performance improvements with the newer STP version.
Looking forward to trying this out.

Going to merge.

@MartinNowack MartinNowack merged commit ca11710 into klee:master Oct 18, 2024
@ccadar
Copy link
Contributor Author

ccadar commented Oct 18, 2024

Thanks, @MartinNowack .
Yes, we should give STP 2.3.4 / master a try -- also, I noticed that 2.3.3 does not compile anymore with newer compilers (might be easy to fix, but I haven't tried it), so we might need to move away from it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MartinNowack MartinNowack MartinNowack approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ccadar @MartinNowack