[drake_cmake_external] Add an upgrade script for dependencies

[tyler-yankee]

Towards #432.

---

This change is 

[tyler-yankee]

+a:@mwoehlke-kitware for feature review, please?
cc: @Aiden2244, feel free to self-assign for review also if you have time

Reviewable status: all discussions resolved, LGTM missing from assignee mwoehlke-kitware, platform LGTM missing (waiting on @mwoehlke-kitware)

[mwoehlke-kitware]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee mwoehlke-kitware, platform LGTM missing

---

private/upgrade_cmake_externals.py line 20 at r1 (raw file):

import re
import shlex
import subprocess

Everything this is being used for can be done (and generally with less code, to boot) within Python.

---

private/upgrade_cmake_externals.py line 37 at r1 (raw file):

def _download_with_sha(*, base_url, filename):

This all seems very unnecessary. Suggestion:

def _get_sha(url):
    """Given a url, downloads it (in memory) and returns the sha256 checksum.
    """
    hasher = hashlib.sha256()
    with urllib.request.urlopen(url) as response:
        while True:
            data = response.read(4096)
            if not data:
                break
            hasher.update(data)
    return hasher.hexdigest()

...and then _run isn't needed either.

---

private/upgrade_cmake_externals.py line 52 at r1 (raw file):

def _check_url(*, url):

This seems entirely pointless.

[mwoehlke-kitware]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee mwoehlke-kitware, platform LGTM missing (waiting on @tyler-yankee)

---

private/upgrade_cmake_externals.py line 73 at r1 (raw file):

    return re.compile(
        r"ExternalProject_Add\s*\(\s*"
        rf"{package}\b[\s\S]*?"

How is [\s\S] different from .?

---

private/upgrade_cmake_externals.py line 80 at r1 (raw file):

if __name__ == "__main__":

It is generally recommended to put the body of a script in a function, and call that function if __name__ == "__main__":. (Consider also calling the function like main(sys.argv[1:]). I like to name the input args and use options = parser.parse_args(args).)

---

private/upgrade_cmake_externals.py line 114 at r1 (raw file):

    old_sha = match.group(2)
    print(f"+ URL: {old_url}")
    print(f"+ SHA256: {old_sha}")

Might as well include "Old" here. Also, "256" isn't terribly necessary, and dropping it (below also) allows everything to line up.

Suggestion:

    print(f"+ Old URL: {old_url}")
    print(f"+ Old SHA: {old_sha}")

---

private/upgrade_cmake_externals.py line 130 at r1 (raw file):

    # Replace the URL and SHA in CMakeLists.txt.
    new_text = re.sub(old_url, new_url, text)

This is a) dodgy, and b) unnecessary. Please use str.replace instead.

[tyler-yankee]

Reviewable status: 6 unresolved discussions, LGTM missing from assignee mwoehlke-kitware, platform LGTM missing (waiting on @mwoehlke-kitware)

---

private/upgrade_cmake_externals.py line 20 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Everything this is being used for can be done (and generally with less code, to boot) within Python.

done

---

private/upgrade_cmake_externals.py line 37 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

This all seems very unnecessary. Suggestion:

def _get_sha(url):
    """Given a url, downloads it (in memory) and returns the sha256 checksum.
    """
    hasher = hashlib.sha256()
    with urllib.request.urlopen(url) as response:
        while True:
            data = response.read(4096)
            if not data:
                break
            hasher.update(data)
    return hasher.hexdigest()

...and then _run isn't needed either.

Thanks! I was borrowing some code from Drake just to get it off the ground with commands I was more familiar with. This greatly simplifies the script.

---

private/upgrade_cmake_externals.py line 52 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

This seems entirely pointless.

done (removed)

---

private/upgrade_cmake_externals.py line 73 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

How is [\s\S] different from .?

You might know better than me; I was under the assumption \s includes \n, while . does not. I'm trying to account for potential line breaks in there.

That particular line of the regex might be a bit verbose.

---

private/upgrade_cmake_externals.py line 80 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

It is generally recommended to put the body of a script in a function, and call that function if __name__ == "__main__":. (Consider also calling the function like main(sys.argv[1:]). I like to name the input args and use options = parser.parse_args(args).)

done

---

private/upgrade_cmake_externals.py line 114 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Might as well include "Old" here. Also, "256" isn't terribly necessary, and dropping it (below also) allows everything to line up.

done

---

private/upgrade_cmake_externals.py line 130 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

This is a) dodgy, and b) unnecessary. Please use str.replace instead.

Derp, thanks. I lost this somewhere in a copy-paste or a prior iteration.

[mwoehlke-kitware]

private/upgrade_cmake_externals.py line 73 at r1 (raw file):
Ah:

In the default mode, this matches any character except a newline. If the DOTALL flag has been specified, this matches any character including a newline.

I wonder if DOTALL would be less surprising?

[mwoehlke-kitware]

with one minor "could be better". (Disclaimer: did not test.)

@mwoehlke-kitware reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, platform LGTM missing (waiting on @tyler-yankee)

---

private/upgrade_cmake_externals.py line 35 at r2 (raw file):

def re_externalproject(package):

Oh, BTW this should probably be _re_externalproject

Suggestion:

def _re_externalproject(package):

[tyler-yankee]

1. One other idea I discussed with @Aiden2244 f2f was doing something similar to Drake's new_release.py here, which is to automatically stage and/or commit the results as [drake_cmake_external] Upgrade <package> to <version>. Would definitely be nice to have, and probably pretty easy, but I think it's best done as a follow-up so that we can at least get this workflow off the ground for next month's upgrades.

2. Re: testing. I've been running it throughout with various packages and versions to explore different cases. I'm not sure adding test code is worth the effort for this little "convenience" script. I have not run an actual build on drake_cmake_external after an update to verify that the shasums are correct, but since we're logging the URLs from which it's obtaining them, I can't possibly see how the script would make up some other value.

3. After this lands, we'll need a one-time PR to get a baseline "up-to-date with Drake"; after that it can happen on a monthly basis only if Drake is updating its versions. I can do that, or @Aiden2244 if you have time?

Reviewable status: all discussions resolved, platform LGTM missing (waiting on @tyler-yankee)

---

private/upgrade_cmake_externals.py line 73 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Ah:

In the default mode, this matches any character except a newline. If the DOTALL flag has been specified, this matches any character including a newline.

I wonder if DOTALL would be less surprising?

With DOTALL replacing [\s\S]* with .* works, and I think it's slightly cleaner. Thanks.

---

private/upgrade_cmake_externals.py line 35 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Oh, BTW this should probably be _re_externalproject

Thanks!

[tyler-yankee]

+a:@sammy-tri for platform review (per schedule), please.

[mwoehlke-kitware]

@mwoehlke-kitware reviewed all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee sammy-tri, platform LGTM missing (waiting on @sammy-tri)

[mwoehlke-kitware]

Possible clarification: "did not test" ⇒ I did not check this out and locally verify that it actually works. That's not a complaint/blocker/whatever, just making note that I am taking your word that it works. (And offering a chance for someone to complain and ask for independent verification.)

Most of our release tooling does not have tests aside from use-in-anger.

Reviewable status: all discussions resolved, LGTM missing from assignee sammy-tri, platform LGTM missing (waiting on @sammy-tri)

[sammy-tri]

-a:@sammy-tri +a:@rpoyner-tri per rotation

Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM missing (waiting on @mwoehlke-kitware and @rpoyner-tri)

[rpoyner-tri]

@rpoyner-tri reviewed 1 of 2 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM missing (waiting on @tyler-yankee)

[rpoyner-tri]

Reviewable status: complete! all discussions resolved, platform LGTM from [rpoyner-tri] (waiting on @tyler-yankee)