Export Drake version in drake-config.cmake

[mwoehlke-kitware]

Improve cmake_configure_file to recognize an --atonly option which replicates CMake's @ONLY mode, refactoring and simplifying the code in the process and also fixing a minor bug where e.g. ${var}} would incorrectly consume both terminators. Modify CMake build to pass the version information into the Bazel build. Add logic to extract the version information (if provided) and to provide it in the generated drake-config.cmake. Use ${CMAKE_FIND_PACKAGE_NAME} consistently in the same.

---

This change is 

[mwoehlke-kitware]

+@jwnimmer-tri for feature review, please.

Release note note: Drake now sets DRAKE_VERSION (and DRAKE_VERSION_MAJOR, etc.) for CMake consumers using find_package(drake).

[jwnimmer-tri]

Checkpoint for now. I'll need to circle back on the regex stuff when I have a bit more time.

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

---

tools/install/libdrake/BUILD.bazel line 51 at r1 (raw file):

    name = "drake_parse_version_stamp",
    srcs = ["stamp_version.txt"],
    outs = ["drake-version.cmake"],

BTW The name "drake-version.cmake" sounds suspiciously like something that might be installed alongside drake, or consumed by users, but really it's just an internal-only tempfile. Could we rename it to be more clear (and/or add more comments)?

Maybe stamp_version.cmake, to underline that it's solely a transmogrification of the txt to cmake spelling of the data?

---

tools/install/libdrake/parse_version.py line 27 at r1 (raw file):

            # Check version format.
            m = re.match(r'^[0-9.]+([+][^ ]+)?', version_full)

This is too strict. Release candidates, for example, could easily have "rc" in the version number.

It's probably sufficient to reply on the other remaining checks (that the split above has ==3 pieces, and the split below has >= 3 pieces). That's good enough to check "syntactically well-formed" which is all we care about here, not the semantics.

---

tools/install/libdrake/parse_version.py line 51 at r1 (raw file):

    else:
        version_full = '.'.join(version_parts)
        version_parts += [0]*4

This too me a while to grok. I think the intention is to pad with trailing zeros for missing parts? That can be a lot more clear:

if len(version_parts) < 4;
    # In the typical case where Drake does not have any TWEAK version,
    # default it to zero.
    version_parts.append(0)

---

tools/workspace/cmake_configure_file.py line 209 at r1 (raw file):

        '--strict', action='store_true')
    parser.add_argument(
        '--atonly', action='store_true')

I imagine that --atonly mode should be flagged as incompatible (fail-fast) with --autoconf mode?

[mwoehlke-kitware]

Okay, I left a few replies inline, but won't be able to get back to this until at least Friday.

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

---

tools/install/libdrake/BUILD.bazel line 51 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW The name "drake-version.cmake" sounds suspiciously like something that might be installed alongside drake, or consumed by users, but really it's just an internal-only tempfile. Could we rename it to be more clear (and/or add more comments)?

Maybe stamp_version.cmake, to underline that it's solely a transmogrification of the txt to cmake spelling of the data?

Sounds reasonable.

---

tools/install/libdrake/parse_version.py line 27 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is too strict. Release candidates, for example, could easily have "rc" in the version number.

It's probably sufficient to reply on the other remaining checks (that the split above has ==3 pieces, and the split below has >= 3 pieces). That's good enough to check "syntactically well-formed" which is all we care about here, not the semantics.

Hmm, possibly this should just reuse the PEP440 check. On that note, however, just splitting on . won't work because any rc component doesn't belong in any of version_parts. I think this needs to be reworked to return both the original string and a tuple of integer parts.

---

tools/install/libdrake/parse_version.py line 51 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This too me a while to grok. I think the intention is to pad with trailing zeros for missing parts? That can be a lot more clear:

if len(version_parts) < 4;
    # In the typical case where Drake does not have any TWEAK version,
    # default it to zero.
    version_parts.append(0)

Right, but I'd want to write that as a while, not if. And that seems like an unnecessarily complicated way of writing it. But a comment would be reasonable.

---

tools/workspace/cmake_configure_file.py line 209 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I imagine that --atonly mode should be flagged as incompatible (fail-fast) with --autoconf mode?

Okay, I forget how to do that offhand but pretty sure you can (and that's it's well documented, I just don't have time to look now).

[jwnimmer-tri]

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

---

tools/install/libdrake/parse_version.py line 27 at r1 (raw file):

I think this needs to be reworked to return both the original string and a tuple of integer parts.

Agreed.

And that point, we might as well always return a 4-tuple as a contract of this function, so that the logic needed to project the full_version string to the CMake 4-tuple is cohesive with the parsing.

... possibly this should just reuse the PEP440 check.

My gut feeling was that doing so would be overkill.

If the goal is to not have "rc" in any of the 4 individual parts, then the r'^[0-9.]+' regex is enough to split out that part. We can ignore any junk that comes afterwards.

---

tools/install/libdrake/parse_version.py line 51 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Right, but I'd want to write that as a while, not if. And that seems like an unnecessarily complicated way of writing it. But a comment would be reasonable.

The main point is that version_parts should not have 7 or 8 elements, 3 or 4 of which are quietly discarded.

---

tools/workspace/cmake_configure_file.py line 209 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Okay, I forget how to do that offhand but pretty sure you can (and that's it's well documented, I just don't have time to look now).

I'd be fine without argparse shenanigans that automate it, i.e., we could just do this ...

if args.atonly and args.autoconf:
    parser.error("Can't do both ...")

... alongside the other checks immediately below.

[mwoehlke-kitware]

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

---

tools/install/libdrake/BUILD.bazel line 51 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Sounds reasonable.

Done.

---

tools/install/libdrake/parse_version.py line 27 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think this needs to be reworked to return both the original string and a tuple of integer parts.

Agreed.

And that point, we might as well always return a 4-tuple as a contract of this function, so that the logic needed to project the full_version string to the CMake 4-tuple is cohesive with the parsing.

... possibly this should just reuse the PEP440 check.

My gut feeling was that doing so would be overkill.

If the goal is to not have "rc" in any of the 4 individual parts, then the r'^[0-9.]+' regex is enough to split out that part. We can ignore any junk that comes afterwards.

...but it's consistent, it's only a few LOC, and I don't think having that check is a bad thing. But, yes, extracting the r'^[0-9.]+' match seems to be the right way to get the parts. Anyway, this has been reworked now.

---

tools/install/libdrake/parse_version.py line 51 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The main point is that version_parts should not have 7 or 8 elements, 3 or 4 of which are quietly discarded.

Moved to _parse_stamp. I did end up using just an if because we're also asserting that at least three parts are present.

[mwoehlke-kitware]

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

---

tools/workspace/cmake_configure_file.py line 209 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'd be fine without argparse shenanigans that automate it, i.e., we could just do this ...

if args.atonly and args.autoconf:
    parser.error("Can't do both ...")

... alongside the other checks immediately below.

Missed this. Done, now.

[jwnimmer-tri]

feature.

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers

---

-- commits line 7 at r2:
BTW Is this really true?

The old match was non-greedy, with a } after the +? so the +? would not consume too much stuff, would it?

Code quote:

  the process and also fixing a minor bug where e.g. '${var}}' would
  incorrectly consume both terminators. Modify CMake build to pass the

---

tools/workspace/cmake_configure_file.py line 40 at r2 (raw file):

        match = pattern.match(line)
        if not match:
            break

BTW Shall we use modern python?

Suggestion:

    while (match := pattern.match(line)) is not None:

---

tools/workspace/cmake_configure_file.py line 120 at r2 (raw file):

# Transform a source code line using autoconf format.
# The 'definitions' provides variable values, just like _transform_cmake above.
def _transform_autoconf(*, line, definitions, strict, atonly):

nit We should probably clarify the invariant here:

Suggestion:

def _transform_autoconf(*, line, definitions, strict, atonly):
    assert atonly == False

[mwoehlke-kitware]

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

---

tools/workspace/cmake_configure_file.py line 40 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Shall we use modern python?

Done

[jwnimmer-tri]

This is looking in fair shape. You can assign to on-call platform review once you're satisfied with it.

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

[mwoehlke-kitware]

+@EricCousineau-TRI for platform review, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform)

---

-- commits line 7 at r2:
Ew. Okay, I must have been testing with non-greedy matching (TIL: adding '?' makes a sub-expression non-greedy). However, the old expression still wasn't working right:

>>> re.match(r'^(.*?)(@[^ ]+?@|\$\{[^ ]+?\})(.*)([\r\n]*)', 'foo ${x${bar}}').groups()
('foo ', '${x${bar}', '}', '')

This version uses greedy matching for the "prefix" (and also for the substitution, but the [^} ] ensures it can only pick up the inner-most }), which means it matches right-to-left instead of left-to-right, and inside-to-outside, so I think it's bug-free. (Excluding the case of e.g. 'foo=${foo}', which a) I believe results in an infinite loop for both versions, b) is clearly pathological, and c) arguably results in an infinite loop as "correct" behavior.)

---

tools/workspace/cmake_configure_file.py line 120 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit We should probably clarify the invariant here:

Done.

[mwoehlke-kitware]

tools/workspace/cmake_configure_file.py line 120 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Done.

FYI, pycodestyle insists on assert not atonly. I'm not convinced that's actually superior in this case. Are you okay with it, or should we # noqa your original suggestion?

[EricCousineau-TRI]

platform

Reviewed 4 of 7 files at r1, 2 of 3 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions