Install more external targets from the Bazel build

[jamiesnape]

There is currently an issue with installing the IPOPT libraries, but they are statically linked into libdrake.so, so hopefully that is not a blocker. Toward #3129.

---

This change is 

[jamiesnape]

+@jwnimmer-tri and +@mwoehlke-kitware for feature review.

[jamiesnape]

FYI @fbudin69500.

[jwnimmer-tri]

a discussion (no related file):
FYI "Refactor the install logic repeated for multiple external targets" seems entirely appropriate to happen prior to or concurrently with this PR. Is this PR's timeliness important enough that we need to do the copypasta this many times for now, only to rip it out again later?

---

Comments from Reviewable

[mwoehlke-kitware]

Review status: 0 of 11 files reviewed at latest revision, 17 unresolved discussions.

---

BUILD.bazel, line 40 at r1 (raw file):

        "@bullet//:install",
        "@eigen//:install",
        "@ipopt//:install",

I wonder if we should be installing optimizers (or collision libraries, for that matter)? Are consumers expected to use these directly? Are any of their headers included via public drake headers? Are any of their libraries needed by (and not statically linked into) libdrake.so?

---

tools/BUILD, line 80 at r1 (raw file):

)

exports_files(

Given the number of times this is happening, can we make this a macro? Our naming is pretty consistent; I think we could get away with export_cps_scripts(["bullet", "eigen", "ipopt", ...]), implemented something like:

def export_cps_scripts(packages):
    for package in packages:
        exports_files("%s-create-cps.py" % package,
                      visibility=["@%s//:__pkg__" % package])

(...with proper formatting, and feel free to use a different name.)

---

tools/bullet-create-cps.py, line 14 at r1 (raw file):

  "Description": "Real-time collision detection and multi-physics simulation",
  "License": ["Zlib"],
  "Version": "%s",

Should there be a Compat-Version? (Note: no Compat-Version implies compatibility is exact-version only.)

Same for other externals...

---

tools/bullet-create-cps.py, line 17 at r1 (raw file):

  "Default-Components": [
      ":BulletCollision",
      ":LinearMath"

BTW, this is technically superfluous, since it is required by BulletCollision. (If you want it for other reasons, that's fine.)

---

tools/bullet.BUILD, line 20 at r1 (raw file):

)

cc_library(

Pedantic: since BulletCollision depends on this, shouldn't it appear first in the file?

---

tools/bullet.BUILD, line 37 at r1 (raw file):

)

# TODO(jamiesnape): Refactor the install logic repeated for multiple external

Yeah... per #6144 (comment) we should probably do this... suggestion was:

NAME_TBD(
    package = "Bullet",
    script = "@//tools:bullet-create-cps.py",
    version_file = "VERSION",
    dest = "lib/cmake/bullet",
)

...although it occurs to me now that I believe it is safe to assume that dest is "lib/cmake/%s" % package.lower(), so that's one less parameter needed.

---

tools/bullet.BUILD, line 50 at r1 (raw file):

    name = "cps",
    srcs = ["VERSION"],
    outs = ["bullet.cps"],

This should match the CPS Name (IOW s.b. Bullet.cps, not bullet.cps).

Creating a macro would help avoid this sort of mismatch (the .cps and Config.cmake would all use the same placeholder).

---

tools/bullet.BUILD, line 81 at r1 (raw file):

        "BulletConfigVersion.cmake",
    ],
    strip_prefix = ["**/"],

This should be unnecessary here. (If it isn't, partial path determination isn't working as intended.)

---

tools/bullet.BUILD, line 89 at r1 (raw file):

    guess_hdrs = "PACKAGE",
    hdr_dest = "include/bullet",
    docs = ["AUTHORS.txt"],

Have you have run tools/buildifier.sh on this? (The order here looks suspiciously unlike what I'd expect from buildifier...)

---

tools/ipopt.BUILD, line 148 at r1 (raw file):

    name = "cps",
    srcs = ["Ipopt/src/Common/config_ipopt_default.h"],
    outs = ["ipopt.cps"],

Again, s.b. IPOPT.cps...

---

tools/ipopt.BUILD, line 179 at r1 (raw file):

        "IPOPTConfigVersion.cmake",
    ],
    strip_prefix = ["**/"],

Again, superfluous.

---

tools/nlopt-create-cps.py, line 6 at r1 (raw file):

import sys

def_re = re.compile("#define ([^\s]+_VERSION)\s+([^\s]+)")

BTW... seriously, nlopt's version symbols are e.g. MAJOR_VERSION with no 'nlopt'?!

/me looks

Yes. Yes they are. 😱

---

tools/nlopt.BUILD, line 136 at r1 (raw file):

    name = "cps",
    srcs = ["nlopt_config.h"],
    outs = ["nlopt.cps"],

Sensing a pattern yet? 😄

---

tools/nlopt.BUILD, line 167 at r1 (raw file):

        "NLoptConfigVersion.cmake",
    ],
    strip_prefix = ["**/"],

Same...

---

tools/pybind11-create-cps.py, line 30 at r1 (raw file):

    }
  },
  "X-CMake-Includes": ["${CMAKE_CURRENT_LIST_DIR}/pybind11Tools.cmake"]

Okay... this is correct, but I was surprised that pybind11Config.cmake just includes this directly.

---

tools/pybind11.BUILD, line 84 at r1 (raw file):

        "tools/pybind11Tools.cmake",
    ],
    strip_prefix = ["**/"],

BTW, here this is correct. (Also, the macro refactor either needs to take extra files and a strip_prefix, or the non-CPS files should be a separate install_files. I lean toward the latter, and rename the config label install_cmake_config.)

---

Comments from Reviewable

[mwoehlke-kitware]

Review status: 0 of 11 files reviewed at latest revision, 15 unresolved discussions.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI "Refactor the install logic repeated for multiple external targets" seems entirely appropriate to happen prior to or concurrently with this PR. Is this PR's timeliness important enough that we need to do the copypasta this many times for now, only to rip it out again later?

See also my reviews. Considering there are some (currently minor, but...) mistakes that a macro would make harder to make, I agree I'd really like to see this refactored.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 4 of 11 files at r1.
Review status: 4 of 11 files reviewed at latest revision, 17 unresolved discussions.

---

BUILD.bazel, line 40 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

I wonder if we should be installing optimizers (or collision libraries, for that matter)? Are consumers expected to use these directly? Are any of their headers included via public drake headers? Are any of their libraries needed by (and not statically linked into) libdrake.so?

I was just looking at that for Bullet. No Drake hdrs need anything from Bullet, its use is buried within a virtual wrapper.

---

tools/bullet.BUILD, line 15 at r1 (raw file):

    hdrs = glob(["src/BulletCollision/**/*.h"]) + ["src/btBulletCollisionCommon.h"],
    copts = ["-Wno-all"],
    defines = ["BT_USE_DOUBLE_PRECISION"],

I am not sure that we should install the Bullet headers, but if we do then we need to make sure the defines here appear in the CPS output so that calling code will have it. I didn't check yet -- do the emitted *.cmake files contain this definition?

---

tools/bullet.BUILD, line 42 at r1 (raw file):

py_binary(
    name = "create-cps",
    srcs = ["@//tools:bullet-create-cps.py"],

Meta It looks like #6190 is going to pass review, so probably new PRs should follow its style and use @drake// instead of @// in lines like this.

---

Comments from Reviewable

[jamiesnape]

Review status: 4 of 11 files reviewed at latest revision, 17 unresolved discussions.

---

tools/bullet.BUILD, line 15 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am not sure that we should install the Bullet headers, but if we do then we need to make sure the defines here appear in the CPS output so that calling code will have it. I didn't check yet -- do the emitted *.cmake files contain this definition?

It will once I bump the pycps version. Whether we install headers, I guess depends on the purpose of the install. My original understanding was that other projects would like to be able to use exactly the versions of a given external that Drake uses, hence you would want headers.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: 4 of 11 files reviewed at latest revision, 17 unresolved discussions.

---

tools/bullet.BUILD, line 15 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

It will once I bump the pycps version. Whether we install headers, I guess depends on the purpose of the install. My original understanding was that other projects would like to be able to use exactly the versions of a given external that Drake uses, hence you would want headers.

So for Bullet, either the cps bump should be added to this PR, or the actual installation should be deferred to a future PR. (The prep work of library factoring could still remain here.)

(Let's move the "Should we install?" discussion to the BUILD.bazel thread that's already started on that topic.)

---

Comments from Reviewable

[jamiesnape]

Review status: 4 of 11 files reviewed at latest revision, 17 unresolved discussions.

---

tools/bullet.BUILD, line 15 at r1 (raw file):

So for Bullet, either the cps bump should be added to this PR, or the actual installation should be deferred to a future PR. (The prep work of library factoring could still remain here.)

I have it ready locally, so it will be on this PR.

---

Comments from Reviewable

[jwnimmer-tri]

BUILD.bazel, line 40 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I was just looking at that for Bullet. No Drake hdrs need anything from Bullet, its use is buried within a virtual wrapper.

On the question on "should we", I haven't thought too hard about it.

I don't think installing Bullet etc. here is an immediate must -- because (1) Drake's use of those libraries is entirely hidden under virtual interfaces, and (2) the immediate priority is to get Director & Spartan working so that we can turn off the duplicated CMake build logic -- and I don't think the wrapped solver engines and collision engines are on that critical path (but correct me if I'm wrong).

However, in the big picture I think installing these libraries is probably appropriate, for the possible case that some user wants to use exactly the same version as Drake in the future. Certainly if a user did need exactly that we'd need to use this solution -- and so writing the code now for it is only a question of priority management.

(Alternatively, keep in mind another solution is to allow the CMake user to build Drake against their version of BulletCollistion -- and in fact we know we'd like to support this in the future also. That might entirely obviate the need to ever install Bazel's build of it -- if you want to know what Bullet is used, build it yourself.)

So in short: I think its fine to add this, but please keep in mind the relative priorities, and IMO getting Director and Spartan is the only thing that matters in the current spiral.

---

Comments from Reviewable

[mwoehlke-kitware]

Review status: 4 of 11 files reviewed at latest revision, 17 unresolved discussions.

---

tools/bullet.BUILD, line 15 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

So for Bullet, either the cps bump should be added to this PR, or the actual installation should be deferred to a future PR. (The prep work of library factoring could still remain here.)

I have it ready locally, so it will be on this PR.

@jwnimmer-tri, thanks; I'd meant to ask if the support for CPS Definitions I recently added was working. (On that note, it looks like the Bullet CPS is still missing the Definitions?)

Aside: the first three Google results for "common package specification" are my pages 😄.

---

Comments from Reviewable

[fbudin69500]

Review status: 3 of 30 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

tools/BUILD, line 80 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Given the number of times this is happening, can we make this a macro? Our naming is pretty consistent; I think we could get away with export_cps_scripts(["bullet", "eigen", "ipopt", ...]), implemented something like:

def export_cps_scripts(packages):
    for package in packages:
        exports_files("%s-create-cps.py" % package,
                      visibility=["@%s//:__pkg__" % package])

(...with proper formatting, and feel free to use a different name.)

A macro would be great for this indeed. It is cumbersome to have to write all the export files individually.

---

tools/bullet.BUILD, line 15 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

@jwnimmer-tri, thanks; I'd meant to ask if the support for CPS Definitions I recently added was working. (On that note, it looks like the Bullet CPS is still missing the Definitions?)

Aside: the first three Google results for "common package specification" are my pages 😄.

👍 for adding Definitions in CPS.

---

Comments from Reviewable

[jamiesnape]

Something strange going on with the previous push. Hold off reviewing for a moment.

[jamiesnape]

Review status: 3 of 13 files reviewed at latest revision, 16 unresolved discussions.

---

tools/bullet-create-cps.py, line 14 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Should there be a Compat-Version? (Note: no Compat-Version implies compatibility is exact-version only.)

Same for other externals...

The spec says that no Compat-Version implies that Version is the lowest compatible version, which would be correct, but is different to what you are staying here.

---

tools/bullet-create-cps.py, line 17 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

BTW, this is technically superfluous, since it is required by BulletCollision. (If you want it for other reasons, that's fine.)

Done.

---

tools/bullet.BUILD, line 15 at r1 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

👍 for adding Definitions in CPS.

Done.

---

tools/bullet.BUILD, line 20 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Pedantic: since BulletCollision depends on this, shouldn't it appear first in the file?

Done.

---

tools/bullet.BUILD, line 37 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Yeah... per #6144 (comment) we should probably do this... suggestion was:

NAME_TBD(
    package = "Bullet",
    script = "@//tools:bullet-create-cps.py",
    version_file = "VERSION",
    dest = "lib/cmake/bullet",
)

...although it occurs to me now that I believe it is safe to assume that dest is "lib/cmake/%s" % package.lower(), so that's one less parameter needed.

Done.

---

tools/bullet.BUILD, line 50 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

This should match the CPS Name (IOW s.b. Bullet.cps, not bullet.cps).

Creating a macro would help avoid this sort of mismatch (the .cps and Config.cmake would all use the same placeholder).

Done.

---

tools/bullet.BUILD, line 81 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

This should be unnecessary here. (If it isn't, partial path determination isn't working as intended.)

Done.

---

tools/bullet.BUILD, line 89 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Have you have run tools/buildifier.sh on this? (The order here looks suspiciously unlike what I'd expect from buildifier...)

Done.

---

tools/ipopt.BUILD, line 148 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Again, s.b. IPOPT.cps...

Done.

---

tools/ipopt.BUILD, line 179 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Again, superfluous.

Done.

---

tools/nlopt.BUILD, line 136 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Sensing a pattern yet? 😄

Done.

---

tools/nlopt.BUILD, line 167 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Same...

Done.

---

Comments from Reviewable

[jamiesnape]

Review status: 3 of 13 files reviewed at latest revision, 15 unresolved discussions.

---

tools/BUILD, line 80 at r1 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

A macro would be great for this indeed. It is cumbersome to have to write all the export files individually.

Not sure if I agree, but if this were a macro, I would probably remove the script argument to cmake_config().

---

Comments from Reviewable

[jamiesnape]

Review status: 3 of 13 files reviewed at latest revision, 15 unresolved discussions.

---

tools/bullet.BUILD, line 42 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Meta It looks like #6190 is going to pass review, so probably new PRs should follow its style and use @drake// instead of @// in lines like this.

Done.

---

Comments from Reviewable

[jamiesnape]

(should be good to review now)

[jwnimmer-tri]

Reviewed 2 of 6 files at r3, 1 of 4 files at r4.
Review status: 5 of 13 files reviewed at latest revision, 17 unresolved discussions.

---

tools/bullet.BUILD, line 22 at r3 (raw file):

    name = "BulletCollision",
    srcs = glob(["src/BulletCollision/**/*.cpp"]),
    hdrs = glob(["src/BulletCollision/**/*.h"]) + ["src/btBulletCollisionCommon.h"],

FYI It'd be nice to find a way to wrap this to <= 80. Maybe a linebreak after the final [ and then see what buildifier says?

---

tools/bullet.BUILD, line 24 at r3 (raw file):

    hdrs = glob(["src/BulletCollision/**/*.h"]) + ["src/btBulletCollisionCommon.h"],
    copts = ["-Wno-all"],
    defines = ["BT_USE_DOUBLE_PRECISION"],

Hmm... this is probably redundant with the one in LinearMath, since defines percolates? If there is some reason to list it in both places, then it should probably turned into a named constant within this BUILD file, for DRY brittleness reasons?

---

tools/bullet.BUILD, line 39 at r4 (raw file):

cmake_config(
    package = "Bullet",
    script = "@//tools:bullet-create-cps.py",

BTW nit Missed @drake// here? Possibly Meta in other pieces of the PR also.

---

Comments from Reviewable

[jamiesnape]

Review status: 4 of 13 files reviewed at latest revision, 16 unresolved discussions.

---

tools/bullet.BUILD, line 22 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI It'd be nice to find a way to wrap this to <= 80. Maybe a linebreak after the final [ and then see what buildifier says?

Done.

---

tools/bullet.BUILD, line 24 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hmm... this is probably redundant with the one in LinearMath, since defines percolates? If there is some reason to list it in both places, then it should probably turned into a named constant within this BUILD file, for DRY brittleness reasons?

Done.

---

tools/bullet.BUILD, line 39 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW nit Missed @drake// here? Possibly Meta in other pieces of the PR also.

Done.

---

Comments from Reviewable

[fbudin69500]

Reviewed 6 of 11 files at r1, 24 of 24 files at r2.
Review status: 8 of 13 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[mwoehlke-kitware]

Reviewed 6 of 11 files at r1, 1 of 24 files at r2, 2 of 6 files at r3, 4 of 4 files at r5.
Review status: 12 of 13 files reviewed at latest revision, 6 unresolved discussions.

---

tools/BUILD, line 80 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Not sure if I agree, but if this were a macro, I would probably remove the script argument to cmake_config().

How would that work? In order to use the script, it has to be made available to the package... Maybe we could instead modify the WORKSPACE macro to copy it, like we do for package.BUILD?

---

tools/bullet-create-cps.py, line 14 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

The spec says that no Compat-Version implies that Version is the lowest compatible version, which would be correct, but is different to what you are staying here.

Er... yes and no? I think logically they are equivalent. The spec says that if you ask for an older version, a newer version is not compatible. But if you ask for a newer version, an older versions is not acceptable either, so the effect is the same as only accepting the exact version. (In any case, the spec is correct; if my logic is wrong, go with the spec and not my previous comment.)

Anyway, the intent was to verify that we intend no compatibility if the user asks for a version older than the actual version.

---

tools/bullet-create-cps.py, line 20 at r4 (raw file):

      "Type": "dylib",
      "Location": "@prefix@/lib/libBulletCollision.so",
      "Definitions": ["BT_USE_DOUBLE_PRECISION"],

This is inconsistent; in Bazel, the def is on LinearMath; here it is on BulletCollision. Probably it should be consistent?

---

tools/install.bzl, line 426 at r5 (raw file):

#END rules
#==============================================================================
#BEGIN macros

BTW, hmm... the original intent was to separate internals from stuff that users would actually call, but sure, I can see the (pedantically correct) distinction. Okay, this WFM.

---

tools/pybind11-create-cps.py, line 30 at r5 (raw file):

    }
  },
  "X-CMake-Includes": ["${CMAKE_CURRENT_LIST_DIR}/pybind11Tools.cmake"]

BTW, what about the pybind11::pybind11 target? Or list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR})?

---

Comments from Reviewable

[jwnimmer-tri]

tools/bullet-create-cps.py, line 20 at r4 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

This is inconsistent; in Bazel, the def is on LinearMath; here it is on BulletCollision. Probably it should be consistent?

I had a draft of the same defect.

Similarly, we need comments in each of the two files that mention this fact to refer to keeping in sync with the other file.

---

Comments from Reviewable

[fbudin69500]

Reviewed 1 of 6 files at r3, 3 of 4 files at r5.
Review status: 12 of 13 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 3 of 11 files at r1, 1 of 24 files at r2, 1 of 6 files at r3, 3 of 4 files at r4, 1 of 4 files at r5.
Review status: 12 of 13 files reviewed at latest revision, 8 unresolved discussions.

---

tools/install.bzl, line 482 at r4 (raw file):

def install_cmake_config(package):
    """Generate installation information for CMake package configuration and
    package version files.

Please explain that the rule name is hard-coded to :install_cmake_config.

---

tools/ipopt-create-cps.py, line 20 at r4 (raw file):

  "Name": "IPOPT",
  "Description": "Interior Point Optimizer",
  "License": "EPL-1.0",

Probably worth noting the METIS license here, since it's still in effect?

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

Comments from Reviewable

[jamiesnape]

Review status: 6 of 13 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

tools/bullet-create-cps.py, line 14 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Er... yes and no? I think logically they are equivalent. The spec says that if you ask for an older version, a newer version is not compatible. But if you ask for a newer version, an older versions is not acceptable either, so the effect is the same as only accepting the exact version. (In any case, the spec is correct; if my logic is wrong, go with the spec and not my previous comment.)

Anyway, the intent was to verify that we intend no compatibility if the user asks for a version older than the actual version.

Indeed, that is the intent.

---

tools/bullet-create-cps.py, line 20 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I had a draft of the same defect.

Similarly, we need comments in each of the two files that mention this fact to refer to keeping in sync with the other file.

Done.

---

tools/install.bzl, line 482 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Please explain that the rule name is hard-coded to :install_cmake_config.

Done.

---

tools/ipopt-create-cps.py, line 20 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Probably worth noting the METIS license here, since it's still in effect?

Done.

---

Comments from Reviewable

[fbudin69500]

Reviewed 7 of 7 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 7 of 7 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[mwoehlke-kitware]

besides a) the one documentation nit, and b) I still would prefer seeing the exports_files stuff as a macro also. (Note: said macro could even live in the same file.)

---

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

tools/bullet-create-cps.py, line 13 at r2 (raw file):

  "Name": "Bullet",
  "Description": "Real-time collision detection and multi-physics simulation",
  "License": ["Zlib"],

Heh... ["ZLib"] and "ZLib" actually have the same meaning, but yeah, for one license, the []s are superfluous 😄.

---

tools/install.bzl, line 482 at r7 (raw file):

def install_cmake_config(package):
    """Generate installation information for CMake package configuration and
    package version files. The rule name is always :install_cmake_config.

Pedantic: :install_cmake_config should probably be styled somehow. (My guess would be double-backticks for "code" styling. I'm not sure if double-quotes are also necessary, but I lean toward "no".)

IOW:

"""Generate installation information for CMake package configuration and
package version files. The rule name is always ``:install_cmake_config``.
...

---

tools/ipopt-create-cps.py, line 20 at r7 (raw file):

  "Name": "IPOPT",
  "Description": "Interior Point Optimizer",
  "License": [

I probably would have left this on one line, but whatever.

BTW, "METIS" is not a license known to SPDX (nor did I spot it in a cursory inspection of the source), but AFAICT "make up an identifier" is TRTTD in such cases.

---

Comments from Reviewable

[jamiesnape]

Review status: 8 of 15 files reviewed at latest revision, 2 unresolved discussions.

---

tools/BUILD, line 80 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

How would that work? In order to use the script, it has to be made available to the package... Maybe we could instead modify the WORKSPACE macro to copy it, like we do for package.BUILD?

Done.

---

tools/install.bzl, line 482 at r7 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Pedantic: :install_cmake_config should probably be styled somehow. (My guess would be double-backticks for "code" styling. I'm not sure if double-quotes are also necessary, but I lean toward "no".)

IOW:

"""Generate installation information for CMake package configuration and
package version files. The rule name is always ``:install_cmake_config``.
...

Done.

---

Comments from Reviewable

[jwnimmer-tri]

---

Reviewed 7 of 7 files at r8.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[mwoehlke-kitware]

---

Reviewed 4 of 7 files at r7, 7 of 7 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

tools/install.bzl, line 482 at r7 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Done.

TY!

---

Comments from Reviewable