[tools] Encode dependency upgrade types in metadata

[tyler-yankee]

Simplify new_release's search heuristics by hard-coding whether it should be looking to upgrade by commit, tag, or release into repository metadata.

Towards #24570.

---

This change is 

[tyler-yankee]

Assigning +a:@mwoehlke-kitware and +a:@jwnimmer-tri for reviews, please, first at a high-level to see if this is the sort of change y'all had in mind.

Jeremy originally mentioned an alternative, more centralized design:

we could have a lookup table in new_release that selects which to use

However, with the existing setup of repository metadata it made more sense to me intuitively to inject the upgrade type metadata at the repository level. I'm happy to hear alternatives.

@tyler-yankee reviewed 66 files and all commit messages, and made 1 comment.
Reviewable status: LGTM missing from assignees mwoehlke-kitware,jwnimmer-tri(platform) (waiting on jwnimmer-tri and mwoehlke-kitware).

[jwnimmer-tri]

Checkpoint on changes to call sites. I haven't looked at the new tooling yet.

@jwnimmer-tri reviewed 63 files and all commit messages, and made 7 comments.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on mwoehlke-kitware and tyler-yankee).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

        repository = "sigmavirus24/github3.py",
        upgrade_type = "tag",
        commit = "3.2.0",

BTW I'm curious why we haven't upgraded to 4.0.1 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "tag" is correct.)

---

tools/workspace/gymnasium_py_internal/repository.bzl line 10 at r1 (raw file):

        repository = "Farama-Foundation/Gymnasium",
        upgrade_type = "release",
        commit = "v1.2.3",

BTW I'm curious why we haven't upgraded to v1.3.0 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "release" is correct.)

---

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

        # of this cohort should be updated at the same time.
        repository = "gazebosim/gz-math",
        upgrade_type = "tag",

BTW As a further improvement on locality, we could imagine putting the tag regex here as well (instead of the dict in new_release)?

---

tools/workspace/libjpeg_turbo_internal/repository.bzl line 24 at r1 (raw file):

        "repository": attr.string(default = "libjpeg-turbo/libjpeg-turbo"),
        "upgrade_type": attr.string(default = "release"),
        "commit": attr.string(default = "2.1.4"),

BTW I'm curious why we haven't upgraded to 3.1.4.1 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "release" is correct.)

---

tools/workspace/libpng_internal/repository.bzl line 8 at r1 (raw file):

    github_archive(
        name = name,
        repository = "glennrp/libpng",

BTW This seems to have been transferred to pnggroup/libpng now.

---

tools/workspace/mpmath_py_internal/repository.bzl line 10 at r1 (raw file):

        repository = "mpmath/mpmath",
        upgrade_type = "release",
        commit = "1.3.0",

BTW I'm curious why we haven't upgraded to 1.4.1 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "release" is correct.)

[tyler-yankee]

Thanks for checking these individually Jeremy, looks like we've maybe spawned some cleanups.

@tyler-yankee made 6 comments and resolved 1 discussion.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail, jwnimmer-tri, mwoehlke-kitware, and tyler-yankee).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I'm curious why we haven't upgraded to 4.0.1 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "tag" is correct.)

It's pinned here:

drake/tools/workspace/new_release.py

Line 101 in 96a3d47

"github3_py_internal": r"^(\d+.)",

fwiw, iirc they've had one major release since and it was a couple years ago, so we're not even that far behind, but we could investigate removing the pin. @mwoehlke-kitware or @BetsyMcPhail, do you have any more context on why this is pinned as such?

---

tools/workspace/gymnasium_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I'm curious why we haven't upgraded to v1.3.0 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "release" is correct.)

It's pinned here:

drake/tools/workspace/new_release.py

Line 87 in 96a3d47

"gymnasium_py_internal": r"v[0-9.]+a[0-9]+",

Interesting. I see its metadata listed in tools/workspace/BUILD.bazel, no commit_pin, no special upgrade rules, and v1.3.0 shouldn't match that regex due to the a.

On this branch and on master we pick up the upgrade:

$ bazel run //tools/workspace:new_release
...
gymnasium_py_internal needs upgrade from v1.2.3 to v1.3.0

@BetsyMcPhail you've been handling the upgrades recently, anything stand out about gymnasium_py_internal?

---

tools/workspace/libjpeg_turbo_internal/repository.bzl line 24 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I'm curious why we haven't upgraded to 3.1.4.1 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "release" is correct.)

I looked at this for a while below too, but commit_pin is 1 immediately below.

That said, with the odd structure of this file, I'm not sure new_release..py's regex matching to replace the commit+sha in the file would actually work.

---

tools/workspace/libpng_internal/repository.bzl line 8 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW This seems to have been transferred to pnggroup/libpng now.

good call...

---

tools/workspace/mpmath_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I'm curious why we haven't upgraded to 1.4.1 yet? Maybe new_release.py isn't finding the metadata file for this external?

(Anyway upgrade_type = "release" is correct.)

#24179

[mwoehlke-kitware]

Do we want to think about any mechanism for detecting that a tag repository has started offering actual releases? Especially as I think we would formerly change over automatically if that happened?

@mwoehlke-kitware reviewed 64 files and all commit messages, and made 3 comments.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail, jwnimmer-tri, and tyler-yankee).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

It's pinned here:

drake/tools/workspace/new_release.py

Line 101 in 96a3d47

"github3_py_internal": r"^(\d+.)",

fwiw, iirc they've had one major release since and it was a couple years ago, so we're not even that far behind, but we could investigate removing the pin. @mwoehlke-kitware or @BetsyMcPhail, do you have any more context on why this is pinned as such?

Hmm... GitHub returns the tags as v1.4.0, v0.7.1, test, test\_annotated\_tag, 4.0.1, 4.0.0, 3.2.0, 3.1.2, ... which means the script thinks the "latest" tag is v1.4.0.

Filtering might help, but this is at least the second repository to have this issue; methinks we may need to rethink our reliance on tags being in order.

---

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW As a further improvement on locality, we could imagine putting the tag regex here as well (instead of the dict in new_release)?

Seconded. I had the same thought while poking into what's going on with github3 not finding new releases.

[BetsyMcPhail]

tools/workspace/gymnasium_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

It's pinned here:

drake/tools/workspace/new_release.py

Line 87 in 96a3d47

"gymnasium_py_internal": r"v[0-9.]+a[0-9]+",

Interesting. I see its metadata listed in tools/workspace/BUILD.bazel, no commit_pin, no special upgrade rules, and v1.3.0 shouldn't match that regex due to the a.

On this branch and on master we pick up the upgrade:

$ bazel run //tools/workspace:new_release
...
gymnasium_py_internal needs upgrade from v1.2.3 to v1.3.0

@BetsyMcPhail you've been handling the upgrades recently, anything stand out about gymnasium_py_internal?

That was user error last month, I confirmed from my notes that it was flagged last month (but not April, as expected). It is also picked up in this month's updates.

[jwnimmer-tri]

For an upstream that has tag later switching to release, I don't think that's very important -- every release always has an associated tag anyway, so we would still see it and offer the upgrade. If upstream starts publishing pre-releases that we accidentally pick up (where only the release meta-data indicates that it's a pre-release), that would show and and we'd need to switch our flag, but aside from that it doesn't seem like a problem to be on the wrong track in that direction.

The more notable hazard is if we're using release and upstream stops using GH releases and only publishes tags. (For example, if they move to GitLib and just keep around GH as a git mirror.) We wouldn't notice that until someone manually went and noticed that we'd missed all updates. On the other hand, if our CI is happy, perhaps the old version was mostly OK and it's not a big deal to keep using it. "Tree falls in a forest" yada yada.

@jwnimmer-tri made 5 comments and resolved 1 discussion.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail and tyler-yankee).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Hmm... GitHub returns the tags as v1.4.0, v0.7.1, test, test\_annotated\_tag, 4.0.1, 4.0.0, 3.2.0, 3.1.2, ... which means the script thinks the "latest" tag is v1.4.0.

Filtering might help, but this is at least the second repository to have this issue; methinks we may need to rethink our reliance on tags being in order.

Ah, okay, so we constrained it to not upgrade past the major version automatically (since I suppose there might be API changes we care about). I'm fine with that approach (not upgrading past 3.x), but it is another argument towards putting the constraint into this file instead of the new_release dict?

---

tools/workspace/gymnasium_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

That was user error last month, I confirmed from my notes that it was flagged last month (but not April, as expected). It is also picked up in this month's updates.

OK, we can call this discussion thread good enough, then.

---

tools/workspace/libjpeg_turbo_internal/repository.bzl line 24 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

I looked at this for a while below too, but commit_pin is 1 immediately below.

That said, with the odd structure of this file, I'm not sure new_release..py's regex matching to replace the commit+sha in the file would actually work.

Perhaps not a change to undertake in this pull request, but even if we can't fully automate the upgrade, we should still be reporting than an upgrade is available and give the developer manual instructions for how to do it. That's more along the lines of upgrade_advice instead of a pin, though it could also plausibly an upgrade_type = "manual" or something.

---

tools/workspace/mpmath_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

#24179

Right. Possibly it would make your life easier to annotate that with a commit_pin and/or upgrade_advice here?

[tyler-yankee]

@tyler-yankee made 4 comments.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail, jwnimmer-tri, and mwoehlke-kitware).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

methinks we may need to rethink our reliance on tags being in order.

(For context from last ~week offline, we found while debugging with #24570 that new_release finds some junk tags on dm_control_internal after the latest 1.0.41, in a manner similar to the above example with github3_py_internal.)

it is another argument towards putting the constraint into this file instead of the new_release dict?

Yep.

---

tools/workspace/gymnasium_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK, we can call this discussion thread good enough, then.

Ah, no problem. thanks!

---

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Seconded. I had the same thought while poking into what's going on with github3 not finding new releases.

OK, I'd be good with this, though perhaps a separate PR.

---

tools/workspace/libjpeg_turbo_internal/repository.bzl line 24 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Perhaps not a change to undertake in this pull request, but even if we can't fully automate the upgrade, we should still be reporting than an upgrade is available and give the developer manual instructions for how to do it. That's more along the lines of upgrade_advice instead of a pin, though it could also plausibly an upgrade_type = "manual" or something.

agreed

[mwoehlke-kitware]

Okay. I just wanted to call it out to make sure we'd thought about it.

@mwoehlke-kitware made 2 comments.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail, jwnimmer-tri, and tyler-yankee).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

methinks we may need to rethink our reliance on tags being in order.

(For context from last ~week offline, we found while debugging with #24570 that new_release finds some junk tags on dm_control_internal after the latest 1.0.41, in a manner similar to the above example with github3_py_internal.)

it is another argument towards putting the constraint into this file instead of the new_release dict?

Yep.

The problem is that the "latest" non-ignored release is v1.4.0, which isn't newer than 3.2.0. (As noted, something similar happened with dm_control_internal.) Releases should be in order, tags I believe are nominally in reverse lexicographical order. Ignoring tags that start with v is a work-around, but I'd feel better if we had a semi-reliable way to order tags even in the face of inconsistent naming conventions. (OTOH, we investigated that with dm_control_internal and IIRC it wasn't practical for that repository.)

[mwoehlke-kitware]

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

OK, I'd be good with this, though perhaps a separate PR.

Honestly, I'd lean toward making it part of this one. We're already moving the update type from the script to the workspaces; IMHO it makes sense to move the filters at the same time. If nothing else, there is significant overlap/similarity in those two changes.

[tyler-yankee]

@tyler-yankee made 2 comments.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

The problem is that the "latest" non-ignored release is v1.4.0, which isn't newer than 3.2.0. (As noted, something similar happened with dm_control_internal.) Releases should be in order, tags I believe are nominally in reverse lexicographical order. Ignoring tags that start with v is a work-around, but I'd feel better if we had a semi-reliable way to order tags even in the face of inconsistent naming conventions. (OTOH, we investigated that with dm_control_internal and IIRC it wasn't practical for that repository.)

Right, the problem you run into here is having to grab the date associated with each individual tag, which gets real expensive as you hammer the GH API.

---

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Honestly, I'd lean toward making it part of this one. We're already moving the update type from the script to the workspaces; IMHO it makes sense to move the filters at the same time. If nothing else, there is significant overlap/similarity in those two changes.

Fair enough! (that was just my first instinct upon seeing the comment, I have yet to sketch any of the requested changes out)

[mwoehlke-kitware]

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Right, the problem you run into here is having to grab the date associated with each individual tag, which gets real expensive as you hammer the GH API.

...which is the wrong approach. IIRC, however, we did try to sort by version, but that didn't go any better? I wonder if it would go better if, instead of black-listing, we white-list what tags are allowed (and only try to sort those that pass). The down-side of course is that that risks missing updates if the convention changes.

Alternatively, there is theoretically graphql...

[tyler-yankee]

@tyler-yankee made 1 comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

however, we did try to sort by version, but that didn't go any better?

Trying to semver sort tags generically is less trivial than one may imagine. Some repos have their package name in the tag, I imagine there are other bad patterns that we'd have to work around, or good patterns that we'd struggle to capture.

[tyler-yankee]

@tyler-yankee made 1 comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail and jwnimmer-tri).

---

a discussion (no related file):
working

@jwnimmer-tri should this PR also tweak the Bazel buildifier lint to enforce proper ordering of the new upgrade_type argument?

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on BetsyMcPhail and tyler-yankee).

---

a discussion (no related file):

Previously, tyler-yankee (Tyler Yankee) wrote…

working

@jwnimmer-tri should this PR also tweak the Bazel buildifier lint to enforce proper ordering of the new upgrade_type argument?

Yes, if the tables json needs an adjustment for new arg names, the PR that adds the new names is the place to do it.

[mwoehlke-kitware]

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

however, we did try to sort by version, but that didn't go any better?

Trying to semver sort tags generically is less trivial than one may imagine. Some repos have their package name in the tag, I imagine there are other bad patterns that we'd have to work around, or good patterns that we'd struggle to capture.

That's where a whitelist regex might help; the regex can capture/extract the sortable part of the version string?

[jwnimmer-tri]

@jwnimmer-tri made 1 comment and resolved 1 discussion.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on tyler-yankee).

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

That's where a whitelist regex might help; the regex can capture/extract the sortable part of the version string?

We can leave this one alone. We've explicitly pinned it to 3.x and that's okay for now.

[tyler-yankee]

@tyler-yankee made 1 comment and resolved 1 discussion.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on tyler-yankee).

---

tools/workspace/mpmath_py_internal/repository.bzl line 10 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Right. Possibly it would make your life easier to annotate that with a commit_pin and/or upgrade_advice here?

=> #24594

[tyler-yankee]

@tyler-yankee made 1 comment and resolved 1 discussion.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on tyler-yankee).

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes, if the tables json needs an adjustment for new arg names, the PR that adds the new names is the place to do it.

TIL we don't, and can't, lint keyword argument order in repository.bzl (or any *.bzl file for that matter) files? Feel free to correct if I'm misunderstanding, but that seems to be the behavior I've discovered locally, and supported by some searching. Real unfortunate, as I'd like the way we invoke github_archive (a lot of places!) to be consistent, but 🤷

[tyler-yankee]

@tyler-yankee reviewed 6 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on jwnimmer-tri and mwoehlke-kitware).

---

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Fair enough! (that was just my first instinct upon seeing the comment, I have yet to sketch any of the requested changes out)

Implemented as tags_pattern, PTAL.

---

tools/workspace/libpng_internal/repository.bzl line 8 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

good call...

=> #24601 (will rebase as needed)

[tyler-yankee]

@tyler-yankee made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on jwnimmer-tri and mwoehlke-kitware).

---

tools/workspace/libjpeg_turbo_internal/repository.bzl line 24 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

agreed

=> #24603

That leaves the question of what to do with the upgrade_type here. I don't think it much matters, at least not with the current machinery? "Scripted" upgrades like crate_universe and now this one have their upgrade types ignored, the script handles it. (Note that for doxygen_internal, which is a "check for upgrades but we don't have an automated script," we do have upgrade_type = "tag" to check GH for new tags.)

[jwnimmer-tri]

@jwnimmer-tri reviewed 6 files and all commit messages, made 6 comments, and resolved 1 discussion.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on tyler-yankee).

---

a discussion (no related file):

Previously, tyler-yankee (Tyler Yankee) wrote…

TIL we don't, and can't, lint keyword argument order in repository.bzl (or any *.bzl file for that matter) files? Feel free to correct if I'm misunderstanding, but that seems to be the behavior I've discovered locally, and supported by some searching. Real unfortunate, as I'd like the way we invoke github_archive (a lot of places!) to be consistent, but 🤷

Right, buildifier does argument order linting in BUILD.bazel files but doesn't mess around insides bzl macros. Oh well.

---

tools/workspace/libjpeg_turbo_internal/repository.bzl line 24 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

=> #24603

That leaves the question of what to do with the upgrade_type here. I don't think it much matters, at least not with the current machinery? "Scripted" upgrades like crate_universe and now this one have their upgrade types ignored, the script handles it. (Note that for doxygen_internal, which is a "check for upgrades but we don't have an automated script," we do have upgrade_type = "tag" to check GH for new tags.)

The question is might it be confusing for maintainers if they see an upgrade_type given, but that type is ignored because of a commit pin? Plausibly the github logic should fail-fast in case a developer tries to add an upgrade_type to a pinned repository, because doing so is nonsense.

Or alternatively, we could have upgrade_type = "pinned" in lieu of commit_pin = 1 so it's consolidated into a single concept.

---

tools/workspace/new_release.py line 86 at r2 (raw file):

# For these repositories, ignore any tags that match the specified regex.
_IGNORED_TAGS = {

I haven't looked carefully, but why aren't these also changed to use tags_pattern now, too?

---

tools/workspace/github.bzl line 43 at r2 (raw file):

            newer versions.
        tags_pattern: optional string specifies a regex describing the
            portion of the current tag in use to lock as invariant (achieved by

nit The extra words are confusing (easy to mis-parse the grouping of grammar).

Suggestion:

current tag

---

tools/workspace/github.bzl line 46 at r2 (raw file):

            a parenthetical group). This can be used to pin to a given major or
            major.minor release series. For example, if version 1.2 of a
            package is currently in use and `tags_pattern = r"^(\\d+.)"`

nit I think we need to escape the . in this sample regex?

---

tools/workspace/github.bzl line 314 at r2 (raw file):

    # Create a summary file for Drake maintainers.
    generate_repository_metadata(

The bzl logic for all of these repository rules (which I believe all flows down into here) needs to validate that upgrade_type is one of the allowed choices.

[tyler-yankee]

@tyler-yankee made 3 comments.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on jwnimmer-tri, mwoehlke-kitware, and tyler-yankee).

---

tools/workspace/github.bzl line 46 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I think we need to escape the . in this sample regex?

yep, I missed the extra escape when converting it from a raw string 🙂

---

tools/workspace/new_release.py line 86 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I haven't looked carefully, but why aren't these also changed to use tags_pattern now, too?

I can look at converting these too. A couple considerations:

• We can drop libpng_internal and sdformat_internal here if the "is prerelease" check didn't warn when skipping prereleases; these are here because they reduce noise for these repos which have many such tags. WDYT (also @mwoehlke-kitware)?
• To clarify, was the original motivation for moving these extras (pinned tags series, ignored tags) out of new_release just that it co-locates the information with the packages themselves rather than the upgrade script, so it's clearer for maintainers to see on a per-package level? What I'm concerned about is the flipside that we're decoupling data and logic for something that really is coupled by design.

---

tools/workspace/libjpeg_turbo_internal/repository.bzl line 24 at r1 (raw file):

but that type is ignored because of a commit pin ... so it's consolidated into a single concept

To me these are two distinct concepts, and ignoring under a pin is intuitive at least to me. upgrade_type says how to look for upgrades, commit_pin says "don't upgrade me for now". In particular I'm imagining the case where we temporarily have to pin something, and then upon removing the pin we have to go back to GH and ask "how do they release again?"

I think the case I raised above with scripted upgrades is a different question. In that case "where to look for upgrades" doesn't make sense because the answer is "the script knows how to look for upgrades by itself."

[jwnimmer-tri]

@jwnimmer-tri made 1 comment and resolved 1 discussion.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on mwoehlke-kitware and tyler-yankee).

---

tools/workspace/new_release.py line 86 at r2 (raw file):

To clarify, was the original motivation for moving these extras (pinned tags series, ignored tags) out of new_release just that it co-locates the information with the packages themselves rather than the upgrade script, so it's clearer for maintainers to see on a per-package level?

Yes, and in particular so that when someone looks in the repository.bzl file and sees a stale version number, the answer as to why its stale is right there next to it.

What I'm concerned about is the flipside that we're decoupling data and logic for something that really is coupled by design.

I can't see any perspective in which this is "coupled by design". Could you elaborate?

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on mwoehlke-kitware and tyler-yankee).

---

tools/workspace/new_release.py line 86 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

To clarify, was the original motivation for moving these extras (pinned tags series, ignored tags) out of new_release just that it co-locates the information with the packages themselves rather than the upgrade script, so it's clearer for maintainers to see on a per-package level?

Yes, and in particular so that when someone looks in the repository.bzl file and sees a stale version number, the answer as to why its stale is right there next to it.

What I'm concerned about is the flipside that we're decoupling data and logic for something that really is coupled by design.

I can't see any perspective in which this is "coupled by design". Could you elaborate?

Other the other hand, when I wrote "why aren't these also changed to use tags_pattern now" I had misapprehended that these were positive-selection tag patterns (like the ones we already relocated, below), rather than negative-selection tag patterns. It would be OK to defer moving the negative-selection patterns as future work with a TODO, so that we can proceed with the 7-day changes sooner than later.

[tyler-yankee]

Hm, maybe I was running on a stale cache earlier? In any case the list of exclude_tags_pattern's is down to 1 for now (ros_xacro_internal).

@tyler-yankee reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: LGTM missing from assignee mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on tyler-yankee).

[tyler-yankee]

@tyler-yankee made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on tyler-yankee).

---

a discussion (no related file):
working

the list … is down to 1

or rather, the _IGNORED_TAGS was also applying to the tags associated with a release, in which case upgrade_type is allowed to be “release” when that pattern is specified, too.

[tyler-yankee]

@tyler-yankee reviewed 4 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: LGTM missing from assignee mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on tyler-yankee).

---

a discussion (no related file):

Previously, tyler-yankee (Tyler Yankee) wrote…

working

the list … is down to 1

or rather, the _IGNORED_TAGS was also applying to the tags associated with a release, in which case upgrade_type is allowed to be “release” when that pattern is specified, too.

restored!

[jwnimmer-tri]

@jwnimmer-tri partially reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on tyler-yankee).

---

tools/workspace/new_release.py line 207 at r3 (raw file):

                    break
        return old_commit, new_commit
    elif upgrade_type == "release":

Now that the "release" upgrade_type allows both types of tags specification (include and exclude), the code here needs to actually obey it.

[tyler-yankee]

@tyler-yankee made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on jwnimmer-tri).

---

tools/workspace/new_release.py line 207 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Now that the "release" upgrade_type allows both types of tags specification (include and exclude), the code here needs to actually obey it.

After starting to sketch this out locally, it turns out to be easy for exclude_tags_pattern, far less so for include_tags_pattern. Note that supporting the former and not the latter for releases is actually the case on master (as _IGNORED_TAGS and _OVERLOOK_RELEASE_REPOSITORIES, respectively).

However, semantically I can see the argument for wanting include_tags_pattern for releases. WDYT about kicking that half to another PR, and just fixing the exclude_tags_pattern here? This PR is already decently substantial as-is, and isn't meant to implement a behavior change.

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on tyler-yankee).

---

tools/workspace/new_release.py line 207 at r3 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

After starting to sketch this out locally, it turns out to be easy for exclude_tags_pattern, far less so for include_tags_pattern. Note that supporting the former and not the latter for releases is actually the case on master (as _IGNORED_TAGS and _OVERLOOK_RELEASE_REPOSITORIES, respectively).

However, semantically I can see the argument for wanting include_tags_pattern for releases. WDYT about kicking that half to another PR, and just fixing the exclude_tags_pattern here? This PR is already decently substantial as-is, and isn't meant to implement a behavior change.

I am OK not adding new features. However, we do need to fail-fast when the metadata requests a feature that is not implemented. So if include pattern isn't implemented, then the fail-fast needs to fail when it's used.

[tyler-yankee]

@tyler-yankee reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware (waiting on jwnimmer-tri).

---

tools/workspace/new_release.py line 207 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am OK not adding new features. However, we do need to fail-fast when the metadata requests a feature that is not implemented. So if include pattern isn't implemented, then the fail-fast needs to fail when it's used.

Done.

[jwnimmer-tri]

@jwnimmer-tri reviewed 3 files and all commit messages, and resolved 1 discussion.
Reviewable status: LGTM missing from assignee mwoehlke-kitware (waiting on tyler-yankee).

[tyler-yankee]

@drake-jenkins-bot linux-noble-unprovisioned-gcc-bazel-experimental-mirror-to-s3 please.

[mwoehlke-kitware]

@mwoehlke-kitware reviewed 7 files and made 7 comments.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee mwoehlke-kitware (waiting on tyler-yankee).

---

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Implemented as tags_pattern, PTAL.

Minor, but I'd somewhat prefer tag_pattern over include_tag_pattern. The latter might imply adding to (+=) the set of allowed tags, whereas the intended meaning is to define (=) the set of allowed tags.

---

tools/workspace/new_release.py line 149 at r7 (raw file):

    development_stages = ["alpha", "beta", "pre", "rc", "unstable"]
    if any(stage in commit for stage in development_stages):
        # Heuristically looks like a pre-release; do so quietly so we don't

Grammar: what are we doing quietly? Probably should read "...pre-release; ignore it quietly..."?

---

tools/workspace/new_release.py line 201 at r7 (raw file):

                        tag.name,
                        workspace_name,
                        exclude_pattern=exclude_tags_pattern,

(...because it seems gratuitous, and for consistency with below...)

Suggestion:

exclude_tags_pattern

---

tools/workspace/new_release.py line 218 at r7 (raw file):

    else:
        raise ValueError(
            f"Got upgrade type {upgrade_type} for {workspace_name}; "

(...and maybe consider putting the 'must be one of' in parentheses?)

Suggestion:

Unknown

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r7 (raw file):

        repository = "sigmavirus24/github3.py",
        upgrade_type = "tag",
        include_tags_pattern = "^(\\d+.)",

Digits followed by any character? Did you mean to escape the . here?

(Incidentally, I don't think we use \d anywhere else. Other regexes, at least related to the upgrade script, all seem to use [0-9].)

---

tools/workspace/gz_math_internal/repository.bzl line 13 at r7 (raw file):

        repository = "gazebosim/gz-math",
        upgrade_type = "tag",
        include_tags_pattern = "^(gz)",

BTW, what do the parentheses do here? For that matter, it is also... surprising that the positive matching uses search (making the ^ necessary) while negative matching uses match. I'd prefer sticking .* in the front of any tags_pattern that actually need it.

---

tools/workspace/qhull_internal/repository.bzl line 11 at r7 (raw file):

        upgrade_type = "tag",
        commit = "2020.2",
        include_tags_pattern = "^(2)",

BTW, this is sort of non-obvious

(Also, again, why the parentheses?)

[tyler-yankee]

@tyler-yankee reviewed 19 files and all commit messages, made 6 comments, and resolved 4 discussions.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee mwoehlke-kitware (waiting on mwoehlke-kitware).

---

tools/workspace/new_release.py line 149 at r7 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Grammar: what are we doing quietly? Probably should read "...pre-release; ignore it quietly..."?

Done.

---

tools/workspace/new_release.py line 218 at r7 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

(...and maybe consider putting the 'must be one of' in parentheses?)

Fixed, but FWIW this goes away in the next PR when an Enum replaces it.

---

tools/workspace/github3_py_internal/repository.bzl line 10 at r7 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Digits followed by any character? Did you mean to escape the . here?

(Incidentally, I don't think we use \d anywhere else. Other regexes, at least related to the upgrade script, all seem to use [0-9].)

woops, fixed! (the benefits of r-strings that we now lose...)

---

tools/workspace/gz_math_internal/repository.bzl line 12 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Minor, but I'd somewhat prefer tag_pattern over include_tag_pattern. The latter might imply adding to (+=) the set of allowed tags, whereas the intended meaning is to define (=) the set of allowed tags.

Fair point, I'm good with that

---

tools/workspace/gz_math_internal/repository.bzl line 13 at r7 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

BTW, what do the parentheses do here? For that matter, it is also... surprising that the positive matching uses search (making the ^ necessary) while negative matching uses match. I'd prefer sticking .* in the front of any tags_pattern that actually need it.

drake/tools/workspace/new_release.py

Lines 94 to 98 in 231c260

	# For these repositories, we only look at tags, not releases. For the dict
	# value, use a blank value to match the latest tag or a regex to only select
	# tags that share the match with the tag currently in use; the parentheses
	# group in the regex denotes the portion of the tag to lock as invariant.
	# (This can be used to pin to a given major or major.minor release series.)

This PR is intended to shift this logic to the repository.bzl files. If you'd like to make a follow-up to tweak that behavior I'd be open to it, but IMO we've got enough going on here as-is.

---

tools/workspace/qhull_internal/repository.bzl line 11 at r7 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

BTW, this is sort of non-obvious

(Also, again, why the parentheses?)

Was also unintuitive to me, perhaps Jeremy would have more context but he's out.

https://github.com/qhull/qhull/tags

IIUC locking us on 2020.2 and avoiding the v8.1*, v8.0.2, v7.*, etc....

[mwoehlke-kitware]

@mwoehlke-kitware reviewed 19 files, made 2 comments, and resolved 1 discussion.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee mwoehlke-kitware (waiting on tyler-yankee).

---

tools/workspace/qhull_internal/repository.bzl line 11 at r7 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Was also unintuitive to me, perhaps Jeremy would have more context but he's out.

https://github.com/qhull/qhull/tags

IIUC locking us on 2020.2 and avoiding the v8.1*, v8.0.2, v7.*, etc....

Right... although there isn't an obvious reason to use the date-style releases over the alternative. My thinking was more that "^2" seems like an odd choice as compared to, say, "2[0-9]{3}\.".

---

tools/workspace/new_release.py line 177 at r8 (raw file):

        return old_commit, new_commit
    elif upgrade_type == "tag":
        # Search for the latest tag by default. If an "tags_pattern"

Minor (oversight)... also, this can be rewrapped:

Suggestion:

a "tags_pattern" regex was

[mwoehlke-kitware]

tools/workspace/new_release.py line 177 at r8 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Minor (oversight)... also, this can be rewrapped:

...although, if you want to forgo the re-wrap (and only fix the grammar 'oops') because it makes next line incongruously short, that's okay too...

[tyler-yankee]

@tyler-yankee reviewed 1 file and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware (waiting on mwoehlke-kitware).

---

tools/workspace/new_release.py line 177 at r8 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

...although, if you want to forgo the re-wrap (and only fix the grammar 'oops') because it makes next line incongruously short, that's okay too...

good find, thanks!

---

tools/workspace/qhull_internal/repository.bzl line 11 at r7 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Right... although there isn't an obvious reason to use the date-style releases over the alternative. My thinking was more that "^2" seems like an odd choice as compared to, say, "2[0-9]{3}\.".

Somewhat related to my other thread, I feel like it will be more easily traceable in the history to have this change in a separate commit (even if it's a one-liner, maybe plus a comment). With lots of changed files in this commit, IMO the metadata changes themselves should be mechanical and nothing more.

[tyler-yankee]

@tyler-yankee made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee mwoehlke-kitware (waiting on mwoehlke-kitware and tyler-yankee).

---

a discussion (no related file):
working

Needs local testing ... somehow wiring to upgrade_type got lost?

[tyler-yankee]

@tyler-yankee made 1 comment and resolved 1 discussion.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware (waiting on mwoehlke-kitware).

---

a discussion (no related file):

Previously, tyler-yankee (Tyler Yankee) wrote…

working

Needs local testing ... somehow wiring to upgrade_type got lost?

Nope, apparently Bazel was operating on a stale cache locally and bazel clean did the trick. The PR is fine.

[tyler-yankee]

Merge conflicts resolved. ping @mwoehlke-kitware for :lgtm: - thanks!

@tyler-yankee reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: LGTM missing from assignee mwoehlke-kitware (waiting on tyler-yankee).

[mwoehlke-kitware]

@mwoehlke-kitware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),mwoehlke-kitware (waiting on tyler-yankee).

[tyler-yankee]

@drake-jenkins-bot retest this please