Skip to content
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

CBMC version 6 release process changes #7987

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

NlightNFotis
Copy link
Contributor

This PR contains (temporary) changes while we're orchestrating the new (v6) release of CBMC going forward.

For now, it deactivates pushing build artefacts to third party services, and increments the version number.

@NlightNFotis NlightNFotis added do not merge Version 6 Pull requests and issues requiring a major version bump labels Oct 30, 2023
V6 is currently in flux, with a not-very well established
release process and potentially breaking changes to the binaries.
Avoid pushing experimental releases to third party services while
in preview.
Previously, it would be passed on to CMake which cannot handle
suffixes following a `-`.
Comment on lines +43 to +44
REGEX "CBMC_VERSION = ([0-9.]+).*")
string(REGEX REPLACE "CBMC_VERSION = ([0-9.]+).*" "\\1" CBMC_VERSION ${CBMC_VERSION})
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is to avoid a problem with CMake.

Essentially CMake accepts version of type major[.minor[.patch[.tweak]]] and fails if anything else is fed.

Before we were passing anything specified in config.inc as version and leaving CMake to validate it.

In this way we are just passing the part of the version that is composed of digits and .s, so 6.0.0-preview is considered a valid version and for CMake it is interpreted as 6.0.0 allowing CBMC itself to use the full version specified in config.inc.

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (731338d) 77.72% compared to head (b580d85) 78.37%.
Report is 8 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7987      +/-   ##
===========================================
+ Coverage    77.72%   78.37%   +0.64%     
===========================================
  Files         1701     1701              
  Lines       196386   196386              
===========================================
+ Hits        152647   153923    +1276     
+ Misses       43739    42463    -1276     

see 89 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }}
SLACK_MESSAGE: "${{ job.status == 'success' && 'Homebrew PR submitted successfully' || 'Homebrew PR failed' }}"
run: go run scripts/slack_notification_action.go
# homebrew-pr:
Copy link
Collaborator

Choose a reason for hiding this comment

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

⛏️ It might be good to put a short comment on each of these disabled sections to explain why it is disabled. It is possible that someone might see that they are not receiving new versions from Docker / homebrew and go looking here. So some comments could save some confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will consider this for a next PR, but I want to move this one forward to unblock us.

In the meantime, I'm hoping that someone coming across this change would see the blame long which would lead him to the commit 2d14e99 and its message that explains the situation.

src/config.inc Show resolved Hide resolved

## Stack

* Revert changes to homebrew PR push (`.github/workflows/release-packages.yaml`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commit hashes could be really handy here, assuming that we can avoid invalidating them in a re-base before the PR is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we need commit hashes here - this was supposed to be a casual thing just to verify our reverted changes against the log here, sort of a double-entry bookkeeping practice but for the purpose of managing the process.

But if keen, I can also add this in a new PR.

@kroening
Copy link
Member

kroening commented Nov 1, 2023

Please wait with merging until we've got an agreed plan for version 6.

@thomasspriggs thomasspriggs mentioned this pull request Nov 1, 2023
@thomasspriggs
Copy link
Collaborator

Please wait with merging until we've got an agreed plan for version 6.

@kroening I have now outlined a plan for the process to use for the major release on the Version 6 RFC here - #7743 (comment) I have outlined it on the RFC rather than here, so that the plan along with the discussion around it is straightforward to reference even once PR(s) are merged / closed.

@NlightNFotis
Copy link
Contributor Author

Hello @kroening, may I request another look at this one please?

We're keen to move with the implementation, as we're in a relatively tight timeline with version 6, so if there are no blockers, we want to move with this one and sort out any minor disagreements in follow-up PRs.

@martin-cs
Copy link
Collaborator

We're keen to move with the implementation, as we're in a relatively tight timeline with version 6

Is the time-line and plan for version 6 available anywhere? There are a bunch of interface-breaking changes it would be good to make to goto-analyzer and goto-instrument as part of it.

@TGWDB
Copy link
Contributor

TGWDB commented Nov 13, 2023

We're keen to move with the implementation, as we're in a relatively tight timeline with version 6

Is the time-line and plan for version 6 available anywhere? There are a bunch of interface-breaking changes it would be good to make to goto-analyzer and goto-instrument as part of it.

As raised (a long time ago) in #7743 we'd hoped to be we've been planning this for some time. The exact details of which Version 6 labelled PRs are merged will depend on if they're ready. The actual timeline is ASAP, I'd suggest raising the PR soon if you can. We'd love to start merging PRs and have unstable/pre-releases from... now. (In practice we don't want to cause too much collateral and so have not merged anything breaking yet.)

@NlightNFotis NlightNFotis merged commit e0fa9e3 into diffblue:develop Nov 13, 2023
36 checks passed
@NlightNFotis
Copy link
Contributor Author

Hello, I've merged this one now in the interest of moving the stack of dependent PRs forward (along with the rest of the process).

Feel free to leave any more comments here, or in #7743

This is just the foundation of the rest of the work, so if you find anything objectionable here after the merge, feel free to ask for it and we can raise a new PR to cover that.

@NlightNFotis NlightNFotis deleted the v6_release_process branch November 13, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Version 6 Pull requests and issues requiring a major version bump
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants