openusd_internal: patch reduction progress

[rpoyner-tri]

• enable upgrade.py to work vs OpenUSD dev branch
• steal a symbol hiding trick from patches into the build system

---

This change is 

[rpoyner-tri]

+@jwnimmer-tri for feature review at your leisure.

This set of patches is what I used to get drake to build against the OpenUsd dev branch, which is where upstream patches must be tested and submitted.

Their dev is quite a distance from our reliance on a numbered release, and it pulls in more sub-libraries of OpenUsd. One consequence is we pull a library that invalidates the assumptions of the previously implemented cmake-file scraping code. We can't count on all of the literal file names, etc., living lexically within the pxr_library() invocation any more. Instead, this code uses the cmake interpreter and specially stubbed functions to print out the arguments of interest, after variable expansion. There are still limitations (cmake include statements, the need to stub out whatever is called from CMakeLists.txt files, etc.), but it is a bit more flexible than the prior scheme, and probably could be expanded.

Note that none of this is needed by our currently integrated revision of OpenUsd, so this PR could hang around for a while before we need to merge it.

FWIW, the currently submitted upstream patches are these:
PixarAnimationStudios/OpenUSD#3282
PixarAnimationStudios/OpenUSD#3281
PixarAnimationStudios/OpenUSD#3280
PixarAnimationStudios/OpenUSD#3278

They are slightly different in form than the current drake patches, which are a little rougher and less upstream-friendly.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

[jwnimmer-tri]

LGTM but a few questions.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

tools/workspace/openusd_internal/defs.bzl line 58 at r1 (raw file):

steal a symbol hiding trick from patches into the build system

I'm not sure why this is change is being proposed. We have patches/nanocolor_namespace.patch whose last hunk already seems to set this unconditionally (and that patch is not rm'd as part of this PR), so now we're setting it in two places?

Bigger picture, in my view generally this defs.bzl file is intended to provide for something analogous to the upstream build system (without Drake-specific customizations). The Drake customizations generally should live in patches, not here. But maybe there is a reason to change that posture now?

---

tools/workspace/openusd_internal/upgrade.py line 40 at r1 (raw file):

# argument parsing for pxr_library() must match that file. The remaining
# functions are just no-op stubs as required by the CMakeLists.txt files found
# in the subdirs listed above.

How do you feel about scraping this macro instead of copy-pasting it? I imagine that scraping Public.cmake from the function(pxr_library NAME) needle up through the next ) after the cmake_parse_arguments( needle would be fairly durable?

I suggest that both because it would plausibly adapt automatically when upstream changes their cmake goo in a meaningful way but one that still matches our needles, and because then we don't need to deal with propagating the upstream copyright text into here.

---

tools/workspace/openusd_internal/upgrade.py line 88 at r1 (raw file):

endfunction()

function(pxr_build_test)

BTW I imagine that I would be happier with the readability here if we had an acute list of ...

CMAKE_IGNORED_FUNCTIONS = [
    "pxr_build_test",
    "pxr_test_scripts",
    ....
]

... and then generated the CMake-ignore-this-function boilerplate with a recipe down brlow. It would help us focus on the important point of this content, which is the stuff up above.

---

tools/workspace/openusd_internal/upgrade.py line 113 at r1 (raw file):

    pxr_library() call in the given cmake source code string."""
    script = CMAKE_CODE_STUB + cmake
    with tempfile.NamedTemporaryFile() as tmpf:

nit tmpf invalid abbreviation

---

tools/workspace/openusd_internal/upgrade.py line 116 at r1 (raw file):

        tmpf.write(script.encode("utf8"))
        tmpf.flush()
        cmd = ["cmake", "-P", tmpf.name]

nit cmd invalid abbreviation. (Upstream calls is args, which would be OK here is you like. Or command is fine too.)

---

tools/workspace/openusd_internal/upgrade.py line 120 at r1 (raw file):

            cmd, stderr=subprocess.STDOUT).decode("utf8")

    result = collections.OrderedDict()

nit OrderedDict is vestigial fluff from early-Python days. A vanilla dict() is already ordered.

---

tools/workspace/openusd_internal/upgrade.py line 125 at r1 (raw file):

            continue
        kv = line.split(" = ")
        if len(kv) == 1:

When would len(kv) == 1 ever be true? (This seems like dead code.) Above, we already did a continue when the line did not contain =.

[rpoyner-tri]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

tools/workspace/openusd_internal/defs.bzl line 58 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

steal a symbol hiding trick from patches into the build system

I'm not sure why this is change is being proposed. We have patches/nanocolor_namespace.patch whose last hunk already seems to set this unconditionally (and that patch is not rm'd as part of this PR), so now we're setting it in two places?

Bigger picture, in my view generally this defs.bzl file is intended to provide for something analogous to the upstream build system (without Drake-specific customizations). The Drake customizations generally should live in patches, not here. But maybe there is a reason to change that posture now?

I'll withdraw this shortly. It's probably fine for something like this line to remain a patch. It would be convenient, however for the defs code to take preprocessor definitions (not cmake definitions) as input.

[jwnimmer-tri]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

tools/workspace/openusd_internal/defs.bzl line 58 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

I'll withdraw this shortly. It's probably fine for something like this line to remain a patch. It would be convenient, however for the defs code to take preprocessor definitions (not cmake definitions) as input.

Around line 30 up above we do have some project-wide definitions already (defines = ...), but those are only for things that are global settings.

For settings local to just one library, my premise has been that the CMakeLists are the source of truth, and if we need a different truth then we patch the CMake spelling. Or sometimes, settings are in the C layer instead of the CMake layer, such as in preprocessor #ifndef defaults, in which case the source of truth is the C code so we patch the C code.

In our VTK build I did take the route of using a settings.bzl file as the source of truth for build options, separate from the code, but it's a lot of work to maintain. Therefore, for simpler projects (like USD), keeping the truth directly inline in the source tree is likely to be the most durable option.

That's just practical tactics though, so if we have evidence that a different approach would be a net win, we can certainly consider it. I just don't know what problem you're trying to solve, yet. My mental model of rebasing our patches in preparation for upstreaming them is all of our patches are cherry-picked into their tree and then rebased and cleaned up, all as stacked curated commits on top of their master. If the challenge here was trying to rebase the patches one by one without the others already in evidence, then we shouldn't expect Drake to ever be OK with just a subset of the patches. (Maybe I'm way off base. Just guessing.)

[rpoyner-tri]

Progress. Still working on the cmake scraping and scripting.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

tools/workspace/openusd_internal/defs.bzl line 58 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Around line 30 up above we do have some project-wide definitions already (defines = ...), but those are only for things that are global settings.

For settings local to just one library, my premise has been that the CMakeLists are the source of truth, and if we need a different truth then we patch the CMake spelling. Or sometimes, settings are in the C layer instead of the CMake layer, such as in preprocessor #ifndef defaults, in which case the source of truth is the C code so we patch the C code.

In our VTK build I did take the route of using a settings.bzl file as the source of truth for build options, separate from the code, but it's a lot of work to maintain. Therefore, for simpler projects (like USD), keeping the truth directly inline in the source tree is likely to be the most durable option.

That's just practical tactics though, so if we have evidence that a different approach would be a net win, we can certainly consider it. I just don't know what problem you're trying to solve, yet. My mental model of rebasing our patches in preparation for upstreaming them is all of our patches are cherry-picked into their tree and then rebased and cleaned up, all as stacked curated commits on top of their master. If the challenge here was trying to rebase the patches one by one without the others already in evidence, then we shouldn't expect Drake to ever be OK with just a subset of the patches. (Maybe I'm way off base. Just guessing.)

Done.

[jwnimmer-tri]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

[rpoyner-tri]

Mostly done. Still needs more testing and probably pointy wizard scrutiny of regex usage.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

tools/workspace/openusd_internal/upgrade.py line 40 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

How do you feel about scraping this macro instead of copy-pasting it? I imagine that scraping Public.cmake from the function(pxr_library NAME) needle up through the next ) after the cmake_parse_arguments( needle would be fairly durable?

I suggest that both because it would plausibly adapt automatically when upstream changes their cmake goo in a meaningful way but one that still matches our needles, and because then we don't need to deal with propagating the upstream copyright text into here.

Done.

---

tools/workspace/openusd_internal/upgrade.py line 88 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I imagine that I would be happier with the readability here if we had an acute list of ...

CMAKE_IGNORED_FUNCTIONS = [
    "pxr_build_test",
    "pxr_test_scripts",
    ....
]

... and then generated the CMake-ignore-this-function boilerplate with a recipe down brlow. It would help us focus on the important point of this content, which is the stuff up above.

the list is now built by scraping the macro file; PTAL.

[rpoyner-tri]

Fixed lint stuff and retested builds vs both current drake dep and upstream dev branch.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

[jwnimmer-tri]

feature.

Reviewed 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

---

tools/workspace/openusd_internal/package.BUILD.bazel line 21 at r3 (raw file):

filegroup(
    name = "cmake_macros",
    srcs = glob(["cmake/macros/Public.cmake"]),

nit If this is a glob in order to allow the file to be nominally absent, then we should say allow_empty = True specifically to communicate that intention.

---

tools/workspace/openusd_internal/upgrade.py line 40 at r4 (raw file):

# argument parsing for pxr_library() must match that file. The remaining
# functions are just no-op stubs as required by the CMakeLists.txt files found
# in the subdirs listed above.

nit This comment paragraph is stale.

---

tools/workspace/openusd_internal/upgrade.py line 70 at r4 (raw file):

def _compose_cmake_prelude():
    """Scrape the contents of the cmake/macros/Public.cmake file from OpenUSD.
    Compose a prelude of redefined (mostly stubbed-out) function to use when

typo

Suggestion:

functions

---

tools/workspace/openusd_internal/upgrade.py line 97 at r4 (raw file):

def _interpret(prelude: str, cmake: str):

Having two arguments here doesn't make sense.

Either this function is a low-level helper in which case it takes a simple cmake: str as its sole argument and just runs it (in which case the caller should pre-join the prelude and cmake strings), or else (the more likely option) this function is acutely a NAME=VALUE extractor, in which case the prelude is not actually customizable and should not be an argument.

In other words, it seems to me like the prelude = _compose_cmake_prelude() should happen inside this function, or else I've misunderstood the code and something else needs to be cleared up.

[rpoyner-tri]

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

---

tools/workspace/openusd_internal/package.BUILD.bazel line 21 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit If this is a glob in order to allow the file to be nominally absent, then we should say allow_empty = True specifically to communicate that intention.

🤦 it doesn't even need to be a glob.

---

tools/workspace/openusd_internal/upgrade.py line 97 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Having two arguments here doesn't make sense.

Either this function is a low-level helper in which case it takes a simple cmake: str as its sole argument and just runs it (in which case the caller should pre-join the prelude and cmake strings), or else (the more likely option) this function is acutely a NAME=VALUE extractor, in which case the prelude is not actually customizable and should not be an argument.

In other words, it seems to me like the prelude = _compose_cmake_prelude() should happen inside this function, or else I've misunderstood the code and something else needs to be cleared up.

We don't really need to rebuild the prelude 20 times. Other options are to make prelude a global, or to scoop many of the free functions into a class, so we can manage state properly.

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

---

tools/workspace/openusd_internal/upgrade.py line 97 at r4 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

We don't really need to rebuild the prelude 20 times. Other options are to make prelude a global, or to scoop many of the free functions into a class, so we can manage state properly.

Ah, it's just for performance? Use @functools.cache on the prelude calculator and then we can lose the extra plumbing.

[jwnimmer-tri]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: needs at least two assigned reviewers

[jwnimmer-tri]

+@ggould-tri for platform review per schedule, please.

The context here is that upgrade.py is a maintainer tool run by hand by build system maintainers, when we need to switch our pin of OpenUSD to a newer/different version.

[ggould-tri]

This is all completely mad, but afaict it's correct.

Reviewed 1 of 2 files at r2, 1 of 3 files at r3, 2 of 2 files at r5, all commit messages.
Reviewable status: 4 unresolved discussions

---

tools/workspace/openusd_internal/upgrade.py line 90 at r5 (raw file):

    stubs = "\n\n".join([_expand_stub(function) for function in functions])

    # TODO: scrape out the pxr_library() arg parsing.

nit: TODO should have username or issue number.

---

tools/workspace/openusd_internal/upgrade.py line 92 at r5 (raw file):

    # TODO: scrape out the pxr_library() arg parsing.
    match = re.search(
        r'.*(\bfunction\b *\( *pxr_library.*?cmake_parse_arguments *\(.*?\))',

BTW, author's discretion, this re seems to be correct, but it's brittle; unlike your previous re you aren't explicitly excluding parens from the grouped class, but are relying on the greedy match at the end of the string to guarantee that no parens can sneak in. That works, but the previous approach was both more readable (hah!) and more robust against someone later adding to the end of the re.

Suggestion:

        r'.*(\bfunction\b *\( *pxr_library.*?cmake_parse_arguments *\([^\)]*\))',

---

tools/workspace/openusd_internal/upgrade.py line 107 at r5 (raw file):

    prelude = _compose_cmake_prelude()
    script = "\n\n".join([prelude, cmake])
    with tempfile.NamedTemporaryFile(delete=False) as f:

minor: Consider setting prefix and/or suffix to clarify the file's purpose and semantics.

---

tools/workspace/openusd_internal/upgrade.py line 129 at r5 (raw file):

def _extract(subdir: str) -> dict[str, str | list[str]]:
    """Extracts the pxr_library() call from the given subdir's CMakeLists.txt
    and returns its arguments as more Pythonic data structure. (Refer to the

typo?

Suggestion:

as a more Pythonic data structure

[rpoyner-tri]

Reviewable status: 4 unresolved discussions

---

tools/workspace/openusd_internal/upgrade.py line 90 at r5 (raw file):

Previously, ggould-tri wrote…

nit: TODO should have username or issue number.

actually, it's done. will remove

---

tools/workspace/openusd_internal/upgrade.py line 107 at r5 (raw file):

Previously, ggould-tri wrote…

minor: Consider setting prefix and/or suffix to clarify the file's purpose and semantics.

actually, it's neither. it's the combined whole script. will rename

[ggould-tri]

Reviewable status: 4 unresolved discussions

---

tools/workspace/openusd_internal/upgrade.py line 107 at r5 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

actually, it's neither. it's the combined whole script. will rename

I meant the prefix and suffix kwargs to NamedTemporaryFile; sorry if I was unclear.

[ggould-tri]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),jwnimmer-tri(platform)

[jwnimmer-tri]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),jwnimmer-tri(platform)