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

Update release scripts for staging changes #20532

Merged

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented Nov 13, 2023

Staging jobs now produce packages with version numbers rather than timestamps in the file names. Update release instructions and related scripts accordingly.

Fixes #18300.


This change is Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

Amendment: this actually needs RobotLocomotion/drake-ci#251 to land first. Also, I don't know of any way to test the push_release changes until we've actually dropped artifacts in GitHub.

@mwoehlke-kitware
Copy link
Contributor Author

+@BetsyMcPhail for feature review, please.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)


tools/release_engineering/download_release_candidate.py line 138 at r1 (raw file):

            f"drake-{version[1:]}-cp310-cp310-manylinux_2_31_x86_64.whl",
            f"drake-{version[1:]}-cp311-cp311-manylinux_2_31_x86_64.whl",
            f"drake-{version[1:]}-cp312-cp312-manylinux_2_31_x86_64.whl",

BTW was this removal intentional?

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)


tools/release_engineering/download_release_candidate.py line 138 at r1 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

BTW was this removal intentional?

Follow up, it was added in #20299 and should not be removed

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)

Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

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

Local testing obtained the expected 39 artifacts for test build 0.99.111401:

$ bazel run //tools/release\_engineering:download\_release\_candidate -- --version v0.99.111401
...
$ ls /tmp/drake-release/v0.99.111401/ | wc -l
39

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)


tools/release_engineering/download_release_candidate.py line 138 at r1 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Follow up, it was added in #20299 and should not be removed

Fixed.

Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

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

+@ggould-tri for platform review, please.

BTW, does this need release notes? The next release will have staging .tar.gzs rather than nightly .tar.gzs.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform), missing label for release notes (waiting on @mwoehlke-kitware)

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

(1) This looks good to me on inspection, but I can't run the tool just yet to test it because my machine is overloaded with other stuff. If I don't finish this today I'll get to it tomorrow.
(2) While I don't think we give specific guarantees about our URL formats or filenames, this seems like something that merits a mention in the announcements section of the release notes, in case anyone consumes our release via a script or build rule or something. So you can release notes: none this but add a descriptive sentence for the release notes when that PR opens up or just email the next release engineer.

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform), missing label for release notes (waiting on @mwoehlke-kitware)

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

Reviewed. I believe this PR must also update the release notes template file and the from_binary.md download inventory.

Reviewed 1 of 1 files at r2.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee ggould-tri(platform), missing label for release notes (waiting on @mwoehlke-kitware)


doc/_pages/release_playbook.md line 109 at r2 (raw file):

      has nothing still running (modulo the ``*-coverage`` builds, which we can
      ignore)
   3. Open the latest builds from the following builds:

minor: Is this list still correct? For that matter, is this actually redundant with the --find-git-sha option to the releasetool in step 5?


doc/_pages/release_playbook.md line 148 at r2 (raw file):

      finishing touches in the meantime, but do not merge the release notes nor
      tag the release until all seven builds have succeeded.
3. Update the release notes to have the ``YYYYMMDD`` we choose, and to make

The release notes template will need to be updated to use the new URL format. Once that's done, this step will need to mention updating the version number in the tarball name.


doc/_pages/release_playbook.md line 154 at r2 (raw file):

   them all. There is also a dummy date 2099-12-31 that should also be changed.
   1. Update the github links within ``doc/_pages/from_binary.md`` to reflect
      the upcoming v1.N.0 and YYYYMMDD.

The text this is referencing in from_binary.md also needs to be updated to reflect the changed URLs. Once that's done, this step can drop all mention of YYYYMMDD.


tools/release_engineering/download_release_candidate.py line 10 at r2 (raw file):

  bazel run //tools/release_engineering:download_release_candidate -- \
      --find-git-sha 20220303

minor: This command fails when copied literally; needs a more recent date.
Alternatively, maybe this isn't supposed to work (to prevent overzealous copypasta giving bad results) in which case it should have a note or clearer error message.

Suggestion:

      --find-git-sha 20231113

tools/release_engineering/download_release_candidate.py line 15 at r2 (raw file):

  bazel run //tools/release_engineering:download_release_candidate -- \
      --version v1.0.0

minor: Same as above: Put in something that works, or put in a remark that this isn't expected to work.

@mwoehlke-kitware mwoehlke-kitware added the release notes: fix This pull request contains fixes (no new features) label Nov 14, 2023
Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 unresolved discussions, LGTM missing from assignee ggould-tri(platform)


doc/_pages/release_playbook.md line 109 at r2 (raw file):

Previously, ggould-tri wrote…

minor: Is this list still correct? For that matter, is this actually redundant with the --find-git-sha option to the releasetool in step 5?

Unknown, but since this didn't affect it, I'm going to vote for punting to a new issue if needed.


doc/_pages/release_playbook.md line 148 at r2 (raw file):

Previously, ggould-tri wrote…

The release notes template will need to be updated to use the new URL format. Once that's done, this step will need to mention updating the version number in the tarball name.

Done.


doc/_pages/release_playbook.md line 154 at r2 (raw file):

Previously, ggould-tri wrote…

The text this is referencing in from_binary.md also needs to be updated to reflect the changed URLs. Once that's done, this step can drop all mention of YYYYMMDD.

Done.


tools/release_engineering/download_release_candidate.py line 10 at r2 (raw file):

Previously, ggould-tri wrote…

minor: This command fails when copied literally; needs a more recent date.
Alternatively, maybe this isn't supposed to work (to prevent overzealous copypasta giving bad results) in which case it should have a note or clearer error message.

It's the same date as was present before. Changing it to a more recent, valid date just means it will work for a while before being broken again. Nor has the behavior of the script changed in this respect.

What is the action item here?

  • Do nothing (nothing has changed), at least in this PR.
  • This PR needs to further improve the script to give a better error message.
  • This PR should replace the above with a more obviously 'placeholder' date, e.g. 19990101 or YYYYMMDD.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee ggould-tri(platform)


tools/release_engineering/download_release_candidate.py line 10 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

It's the same date as was present before. Changing it to a more recent, valid date just means it will work for a while before being broken again. Nor has the behavior of the script changed in this respect.

What is the action item here?

  • Do nothing (nothing has changed), at least in this PR.
  • This PR needs to further improve the script to give a better error message.
  • This PR should replace the above with a more obviously 'placeholder' date, e.g. 19990101 or YYYYMMDD.

Hm, I think [YYYYMMDD] is what I'd go with, especially given there's no current sane working case for the analogous line 15.

Staging jobs now produce packages with version numbers rather than
timestamps in the file names. Update release instructions and related
scripts accordingly.
Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform)


tools/release_engineering/download_release_candidate.py line 10 at r2 (raw file):

Previously, ggould-tri wrote…

Hm, I think [YYYYMMDD] is what I'd go with, especially given there's no current sane working case for the analogous line 15.

Done.


tools/release_engineering/download_release_candidate.py line 15 at r2 (raw file):

Previously, ggould-tri wrote…

minor: Same as above: Put in something that works, or put in a remark that this isn't expected to work.

Done.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion

@svenevs svenevs assigned svenevs and unassigned BetsyMcPhail Nov 14, 2023
Copy link
Contributor

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

-@BetsyMcPhail +@svenevs :lgtm_strong:

Reviewed 1 of 3 files at r1, 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Dismissed @BetsyMcPhail from a discussion.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),svenevs

@mwoehlke-kitware mwoehlke-kitware merged commit babae86 into RobotLocomotion:master Nov 14, 2023
1 check passed
@mwoehlke-kitware mwoehlke-kitware deleted the staging-is-versioned branch November 14, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relase downloader should use staging for tgz
4 participants