feat: Add a release script #2848

cpitclaudel · 2022-10-04T23:24:29Z

This removes most of the manual steps in the release process, except for the manual CI workflow trigger, which is left for later.

It also changes the way we do release notes: instead of updating a single file (which creates repeated PR conflicts), each release note item goes into a separate text file numbered after the corresponding PR. The details of this are written up in docs/news/README and a link points to that folder in RELEASE_NOTES.md to avoid confusion.

robin-aws

On a quick glance this looks awesome, very much appreciate the contribution to streamlining the release process!

I will find time soon to review properly, but my high-level strong preference is to not require doing any work on a local machine (fully scripted or not), but to find a way for it to happen in a GitHub Action workflow. There is always risk for local development state to corrupt the release process, sometimes very subtly.

MikaelMayer

Yay, wonderful time-saver script, which will help us release more often I hope.

MikaelMayer · 2022-10-05T16:20:25Z

docs/dev/RELEASE.md

+ check for a run of this workflow on the exact commit to release. (TODO:
+ Run this automatically as part of the prepare-release script.)
+
+3. Run `Scripts/release.py $VER release` from the root of the repository. The


Can you explicitely say to wait till the tests are finished, and how much time it typically last?

MikaelMayer · 2022-10-05T16:22:02Z

docs/dev/RELEASE.md

-8. If preparing a pre-release, stop here, as
- the following steps declare the release as the latest version, which
- is not the intention.
+7. Make a PR in the <https://github.com/dafny-lang/ide-vscode> repository to


Can you change this to "checkout this repository and run the ./publish_script.js" to update the version of Dafny.

docs/dev/news/README.md

Scripts/prepare_release.py

MikaelMayer · 2022-10-05T16:40:24Z

Scripts/prepare_release.py

+ prefix: str # Main version number (1.2.3)
+ rdate: date # Release date
+ suffix: str # Optional marker ("alpha")


I would prefer very much to have complete and explicit names, so that it does not feel like a compiled code :-)

Suggested change

prefix: str # Main version number (1.2.3)

rdate: date # Release date

suffix: str # Optional marker ("alpha")

version_number: str # Main version number (1.2.3)

release_date: date # Release date

version_number_suffix: str # Optional marker ("alpha")

Change rdate. prefix and suffix are complete names: they are fields of the Version class (so do we really need to repeat version?), and more importantly it's not clear with version_number and version_number_suffix whether the suffix is already included in version_number.

In the same answer spirit, it's not clear with "prefix" or "suffix" what of the version numbers are included in which :-)

Maybe the following could reach a consensus?
prefix => major_minor_patch
suffix stays the same.

OK, I renamed prefix to main and suffix to identifier (the name used for this in semantic versioning)

MikaelMayer · 2022-10-05T16:40:45Z

Scripts/prepare_release.py

+ pth.unlink()
+
+class Version(NamedTuple):
+ VERNUM_RE = re.compile("^(?P<prefix>[0-9]+[.][0-9]+[.][0-9]+)(?P<suffix>-.+)?$")


Suggested change

VERNUM_RE = re.compile("^(?P<prefix>[0-9]+[.][0-9]+[.][0-9]+)(?P<suffix>-.+)?$")

VERSION_NUMBER_REGEX = re.compile("^(?P<prefix>[0-9]+[.][0-9]+[.][0-9]+)(?P<suffix>-.+)?$")

Renamed to VERSION_NUMBER_PATTERN, but I find it much harder to read (because the long names hide the rest of the code)

Thanks for the renaming, I think it's appropriate. How does the the long name hide the rest of the code? I'm not sure to understand this part.

Very long function and variable names make code harder to read, because the relevant bits of syntax / control end up further from each other, at least in my experience.

MikaelMayer · 2022-10-05T16:55:11Z

I will find time soon to review properly, but my high-level strong preference is to not require doing any work on a local machine (fully scripted or not, but to find a way for it to happen in a GitHub Action workflow. There is always risk for local development state to corrupt the release process, sometimes very subtly.

I think that, in a first step, mimicking what we used to do manually locally anyway is a good compromise. I tested in on my Windows machine and this script worked.
Later, we can always try to automate more the process and make it purely online, but I think that would be an improvement, not a blocker over this PR.

Co-authored-by: Mikaël Mayer <[email protected]>

robin-aws · 2022-10-05T18:30:19Z

I will find time soon to review properly, but my high-level strong preference is to not require doing any work on a local machine (fully scripted or not, but to find a way for it to happen in a GitHub Action workflow. There is always risk for local development state to corrupt the release process, sometimes very subtly.

I think that, in a first step, mimicking what we used to do manually locally anyway is a good compromise. I tested in on my Windows machine and this script worked. Later, we can always try to automate more the process and make it purely online, but I think that would be an improvement, not a blocker over this PR.

Fair enough, I just need the deeper review to verify we're not doing more locally than before. :)

cpitclaudel · 2022-10-05T18:40:26Z

Fair enough, I just need the deeper review to verify we're not doing more locally than before. :)

Do you want to block this, or are you OK with Mikael's review?

cpitclaudel · 2022-10-05T18:45:57Z

I will find time soon to review properly, but my high-level strong preference is to not require doing any work on a local machine

Agreed, and this script can run from CI as well. The only difficulty would be creating a bot account on github that is allowed to push to the repo.

But since this sort of script invariably takes a few iterations to refine and since it's much easier to debug locally than on CI, I think it's best to start locally.

MikaelMayer · 2022-10-05T21:30:20Z

Scripts/prepare_release.py

- entry = indent(f"- {text}\n{link}", " ").lstrip()
+ for fr in self.fragments[ext]:
+ link = f"(https://github.com/dafny-lang/dafny/pull/{fr.pr})"
+ entry = indent(f"- {fr.contents}\n{link}", " ").lstrip()


I don't think you want to add a newline between the contents and the link. At least in GitHub, where the release nodes can be seen as a formatted markdown file, even though the link seems long, it will actually look like this: #1239

MikaelMayer · 2022-10-05T21:35:48Z

Scripts/prepare_release.py

+ - Improved toaster settings.
+ Dafny will not burn toast again.
+ (https://github.com/dafny-lang/dafny/pull/1234)


Thanks for the testing code ! It helps a lot.

Suggested change

- Improved toaster settings.

Dafny will not burn toast again.

(https://github.com/dafny-lang/dafny/pull/1234)

- Improved toaster settings.

Dafny will not burn toast again (https://github.com/dafny-lang/dafny/pull/1234)

Suggested change

- Improved toaster settings.

Dafny will not burn toast again.

(https://github.com/dafny-lang/dafny/pull/1234)

- Improved toaster settings.

Dafny will not burn toast again.

(https://github.com/dafny-lang/dafny/pull/1234)

See comment below for the formatting.
Besides the extra newline that is not necessarily (have a look at here to see how release notes are currently formatted)I would also add that you would now be forcing users (and me) to do something that we don't want: Add a period before the parentheses.
So, in your example, and in your README, could you be specific about this point? Alternatively (and that would be my favorite method), you could detect the last character of the trimmed string, and if it's a period, you'd remove it.

Done: I've removed the newline. I find that it makes reading the release notes harder, but I won't fight about this.

The existing RELEASE_NOTES are inconsistent on whether to use a period, so I've changed the code to add a period in all cases. The version above with one period for the first sentence and no period for the second doesn't look right at all to me; do you like it better?

I find that it makes reading the release notes harder

I won't argue since you implemented my change, thanks :-) but I'm just curious.
Can you please clarify, is it because you read the source of release notes instead of in a browser? Or is the browser rendering itself harder to read? To me, the first image here:

is much nicer than the alternative that you might find more appealing based on your script:

I find that the second one breaks the flow, it feels like every link is important when the only thing I would like out of release notes is to read them in diagonal.

Regarding two lines: I think we should not even consider the possibility that a feat/fix line in a release notes should have two sentences. I know you added support for it, but it's not something we have done in the past and I don't see a reason why we should embrace those. So if we were to add paragraphs, I'd agree with you, we should end every sentence with a full stop, and make the link its own period-less sentence after that.
However, for a single phrase, with the comment in parentheses containing the link being part of the phrase, there should be no period between the phrase and the parentheses. If there was a period, it should come afterwards.

Now, one more principle to consider: We use bullet points to express each individual release notes, and many bullet points are not complete sentences but just fragments. In this case I think it's conventional wisdom to omit the period at the end of the sentence (after the parentheses containing the link).

Good point. I implemented your suggestion below: no auto punctuation fix and no newline before link, unless there's already multiple lines in the entry.

We can refine the rendering further, probably when we have more data (before the next release)

MikaelMayer

Still one more cosmetic change to make it easier for everyone and ensure we are markdown-friendly.

docs/dev/news/README.md

…lease

docs/dev/news/README.md

Co-authored-by: Mikaël Mayer <[email protected]>

robin-aws

I just reviewed the new workflow and did not look closely at the implementation, will leave that to other reviewers.

On the one hand I definitely love the new approach. On the other I do feel like this is increasing the work that has to happen on a developer's machine, since in the old way we were manually creating the release notes incrementally in each PR. Let me think a bit more on how to avoid that.

robin-aws · 2022-10-07T16:57:08Z

docs/dev/news/README.md

+ Two new toast patterns:
+ - Dafny waterfall logo
+ - Dafny haircut logo
+ (They are the same.)


robin-aws · 2022-10-07T16:58:31Z

docs/dev/RELEASE.md

+ check for a run of this workflow on the exact commit to release. (TODO:
+ Run this automatically as part of the prepare-release script.)


Let's extract the TODOs here into an issue to avoid the distraction for the operator.

I would prefer to keep it here. It's good to be reminded of the issue every time we go through these instructions. But le me know if you disagree and I'll move it.

robin-aws · 2022-10-07T17:26:57Z

docs/dev/RELEASE.md

+6. Create a pull request to merge the newly created branch into `master` (the
+ script will give you a link to do so). Get it approved and merged.


I don't love the idea of releasing from a branch before merging it to mainline. Could we flip the order of steps to do this first, even if it means customizing the GHA that looks for the deep tests? Are we just trying to save the extra delay on waiting for the CI?

Releasing from master forces a freeze to avoid concurrent commits; we've been bitten by that before. What issue do you see with releasing from a branch? Is it the security aspect? (I think we already allow releasing by uploading a custom-packaged zip, so I haven't worried about that part)

I think it's not only pretty common to release from a branch before merging it (that way, it's even possible to cherry-pick bug fixes independently without introducing new features), but in our case, as Clement said, we already encountered the case when we wanted to release after merging with master; however, there was one more commit on master since we started to prepare the release, and this commit was merged unintentionally with the release.
So although I had wished that releases are made from the history of master branches, in practice, this is dangerous.

Okay that's a better reason, I'm happy with this.

robin-aws · 2022-10-07T17:27:33Z

RELEASE_NOTES.md

@@ -1,15 +1,13 @@
 # Upcoming

- fix: Compiled lambdas now close only on non-ghost variables (https://github.com/dafny-lang/dafny/pull/2854)
- fix: Crash in the LSP in some code that does not parse (https://github.com/dafny-lang/dafny/pull/2833)
+See [docs/dev/news/](docs/dev/news/).


I'd add a stronger comment that this file is no longer used as part of the release process.

This is a user-facing file, though, so should we add this type of comments there?
Maybe as a comment that doesn't get rendered?

I don't think we should add a comment about the release process here, but I like the idea of also redirecting users to the folder where the fragments are so that they can see what to expect in the next release.

Ah I missed the fact that the script is still updating this file, I thought it was going to be frozen in this state forever more. Nevermind. :)

robin-aws · 2022-10-07T17:29:28Z

docs/dev/RELEASE.md

+0. Ensure that you are in a clean and up-to-date repository, with the `master`
+ branch checked out and up-to-date and no uncommitted changes.
+
+1. Select a version number `$VER` (e.g., "3.0.0" or "3.0.0-alpha"), then run


Could we have the script infer the correct next version number based on whether there are any new feat or feat! news items? Or at least verify you selected the right version number.

Sure, do you think it's worth it given that we plan to revise our version policy soon?

I'm afraid we would loose the ability to bump the major version number with such an auto-detect 😮
My best suggestion is that:

The script detect feat or feat! extensions

If there are, it suggests to increment the minor version number, and if the user presses ENTER, this is it.

If there are none (only fixes), it suggests to increment the patch version number, and if the user presses ENTER, this is it

But if the user decides to put another version number, it takes it and continue.

This is a lot of work for saving the user from typing 3 numbers ^^ I vote for leaving this for future improvement

MikaelMayer

Removing my "request change"

MikaelMayer

Commenting does not remove the "Request change". Let me approve it.

…lease

cpitclaudel requested a review from MikaelMayer October 4, 2022 23:24

cpitclaudel force-pushed the cpitclaudel_auto-release branch 3 times, most recently from 968d4e5 to fd90a87 Compare October 4, 2022 23:39

feat: Add a release script

6bea8ee

cpitclaudel force-pushed the cpitclaudel_auto-release branch from fd90a87 to 6bea8ee Compare October 4, 2022 23:49

cpitclaudel self-assigned this Oct 4, 2022

robin-aws self-requested a review October 5, 2022 13:54

robin-aws requested changes Oct 5, 2022

View reviewed changes

MikaelMayer requested changes Oct 5, 2022

View reviewed changes

Update docs/dev/news/README.md

97ab685

Co-authored-by: Mikaël Mayer <[email protected]>

Mikael's feedback

329125b

cpitclaudel requested a review from MikaelMayer October 5, 2022 18:43

cpitclaudel added 2 commits October 5, 2022 12:48

Describe format of output and test it

46ac8b9

Rename prefix and suffix to main and identifier

c71ae40

MikaelMayer reviewed Oct 5, 2022

View reviewed changes

MikaelMayer requested changes Oct 5, 2022

View reviewed changes

Remove newlines before links

51d06f5

cpitclaudel force-pushed the cpitclaudel_auto-release branch from 8242e2b to 51d06f5 Compare October 6, 2022 01:02

cpitclaudel requested a review from MikaelMayer October 6, 2022 01:07

MikaelMayer reviewed Oct 6, 2022

View reviewed changes

docs/dev/news/README.md Outdated Show resolved Hide resolved

cpitclaudel added 2 commits October 7, 2022 08:54

Better support for multiline entries

8297bec

Merge remote-tracking branch 'origin/master' into cpitclaudel_auto-re…

3e78fd1

…lease

cpitclaudel mentioned this pull request Oct 7, 2022

feat: Support for arbitrary versions of z3 #2820

Merged

Move release entry from 8.9 to latest

4582480

cpitclaudel requested a review from MikaelMayer October 7, 2022 16:11

cpitclaudel added 4 commits October 7, 2022 09:12

Remove added sentence in RELEASE.md

dee1482

Pacify pylint

30303ab

Check for newlines in trimmed contents

256db39

Add tests

f628020

MikaelMayer reviewed Oct 7, 2022

View reviewed changes

docs/dev/news/README.md Outdated Show resolved Hide resolved

Update docs/dev/news/README.md

14ade58

Co-authored-by: Mikaël Mayer <[email protected]>

cpitclaudel enabled auto-merge (squash) October 7, 2022 16:39

cpitclaudel requested a review from MikaelMayer October 7, 2022 16:40

robin-aws requested changes Oct 7, 2022

View reviewed changes

Delete news before commit

e49a42b

MikaelMayer reviewed Oct 7, 2022

View reviewed changes

MikaelMayer previously approved these changes Oct 7, 2022

View reviewed changes

robin-aws previously approved these changes Oct 11, 2022

View reviewed changes

Merge branch 'master' into cpitclaudel_auto-release

76855a8

cpitclaudel dismissed stale reviews from robin-aws and MikaelMayer via 76855a8 October 11, 2022 23:03

robin-aws previously approved these changes Oct 12, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into cpitclaudel_auto-re…

69d0497

…lease

cpitclaudel dismissed robin-aws’s stale review via 69d0497 October 12, 2022 00:17

robin-aws approved these changes Oct 12, 2022

View reviewed changes

cpitclaudel merged commit 7206268 into master Oct 12, 2022

cpitclaudel deleted the cpitclaudel_auto-release branch October 12, 2022 00:39

	VERNUM_RE = re.compile("^(?P<prefix>[0-9]+[.][0-9]+[.][0-9]+)(?P<suffix>-.+)?$")
	VERSION_NUMBER_REGEX = re.compile("^(?P<prefix>[0-9]+[.][0-9]+[.][0-9]+)(?P<suffix>-.+)?$")

		check for a run of this workflow on the exact commit to release. (TODO:
		Run this automatically as part of the prepare-release script.)

		6. Create a pull request to merge the newly created branch into `master` (the
		script will give you a link to do so). Get it approved and merged.

feat: Add a release script #2848

feat: Add a release script #2848

Conversation

cpitclaudel commented Oct 4, 2022 • edited Loading

robin-aws left a comment • edited Loading

Choose a reason for hiding this comment

MikaelMayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelMayer commented Oct 5, 2022

robin-aws commented Oct 5, 2022

cpitclaudel commented Oct 5, 2022

cpitclaudel commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelMayer left a comment

Choose a reason for hiding this comment

robin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelMayer left a comment

Choose a reason for hiding this comment

MikaelMayer left a comment

Choose a reason for hiding this comment

cpitclaudel commented Oct 4, 2022 •

edited

Loading

robin-aws left a comment •

edited

Loading