Build ibex 2.8.6 from source

[RussTedrake]

Resolves #13219 and #15872
Relevant to #15946

Includes patches to dreal and IbexSolver from @soonhokong.

---

This change is 

[RussTedrake]

+@soonhokong for feature review
+@jwnimmer-tri for platform review, please. (In particular, I'm not sure I've done the licenses properly)

Reviewable status: LGTM missing from assignees soonhokong,jwnimmer-tri(platform), missing label for release notes (waiting on @jwnimmer-tri and @soonhokong)

[jwnimmer-tri]

Checkpoint. I'll come back to the main BUILD file a bit later on.

(In particular, I'm not sure I've done the licenses properly)

FYI The licenses([...]) stanza is BUILD files only serves as developer documentation. It has to functional effect (bazelbuild/bazel#7444). The only thing we really need to care about is how we link & what we install.

Reviewed 9 of 10 files at r1, all commit messages.
Reviewable status: 8 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),soonhokong, missing label for release notes (waiting on @jwnimmer-tri, @RussTedrake, and @soonhokong)

a discussion (no related file):

Includes patches to dreal and IbexSolver from @soonhokong.

I'm not sure exactly what this means, but if any of the content of this PR (as opposed to a git remote that this PR is referring to) was written by Soonho, then please add a Co-authored-by line to the bottom of the commit message, for attribution.

---

---

solvers/test/ibex_solver_test.cc, line 111 at r1 (raw file):

  EXPECT_FALSE(prog_.generic_costs().empty());
  if (solver_.available()) {
    auto result = solver_.Solve(prog_, {});

nit Why is there a , {} here now? It's (still) just the default.

---

solvers/test/ibex_solver_test.cc, line 117 at r1 (raw file):

    const double v1{result.GetSolution(x1)};
    const double v2{result.GetSolution(x2)};
    EXPECT_NEAR(v0, 1.4142135623730951, 1e-8);

nit Should we just use M_SQRT_2 here instead?

---

tools/workspace/dreal/ibex_2.8.6.patch, line 34 at r1 (raw file):

index 365de51..6ea6a12 100644
--- dreal/util/test/box_test.cc
+++ dreal/util/test/box_test.cc

nit To my eye, Drake is not ever building dReal's unit tests, so any patches like this to a unit test would be irrelevant?

Or, if we do need to keep it for some reason, then to comment out code via a patch file always use #if 0 (not //) for improved conflict resolution and re-basing.

---

tools/workspace/ibex/BUILD.bazel, line 11 at r1 (raw file):

package(default_visibility = ["//visibility:public"])

install(

In the parent directory tools/workspace/BUILD.bazel, we need to fix _DRAKE_EXTERNAL_PACKAGE_INSTALLS.

Mostly importantly, the ibex install should not be Ubuntu-only anymore.

But also, ibex should move into the top list (next to ignition_math) so that we install from it's @ibex//:install directly, at which point we should also remove the install stanza from this BUILD file. There's no need to launder the rule through this directory anymore.

---

tools/workspace/ibex/package.BUILD.bazel, line 16 at r1 (raw file):

)

licenses(["notice"])  # LGPL

Replace "notice" with "restricted" and LGPL with LGPL-3.0-only.

FYI by convention in Drake, we use the SPDX identifiers:

https://spdx.org/licenses/LGPL-3.0-only.html

---

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

        # TODO(russt): soonhokong recommends we consider moving this to
        # RobotLocomotion.
        repository = "dreal-deps/ibex-lib",

We strongly discourage building from forks (vs https://github.com/ibex-team/ibex-lib/ upstream), because it makes incorporating upstream upgrades much more difficult. We'd much rather cherry-pick this fork's patches into Drake patch files directly (and apply them with patches = [] in the repository rule).

However, since we can't upgrade past 2.8.6 anytime soon anyway, the immediate impact here is low. Furthermore, at least this github fork will be easier for us to maintain than the current system of a pre-compiled *.deb that this PR is replacing, so at least it's a strict improvement.

Therefore, how about just adding this here:

        # TODO(jwnimmer-tri) Switch to upstream ibex-lib (using local patch
        # files if necessary).

---

tools/workspace/ibex/repository.bzl, line 13 at r1 (raw file):

        # RobotLocomotion.
        repository = "dreal-deps/ibex-lib",
        # As discussed in #15872, we need ibex <= 2.8.7 for CLP support.

nit Typo -- I think we need < 2.8.7 not <= 2.8.7?

[jwnimmer-tri]

Thanks for doing this -- it will make our life on the build & release front much easier over time.

Let me know if you'd like me to push fixes for any of my discussions.

Reviewed 1 of 10 files at r1.
Reviewable status: 20 unresolved discussions, LGTM missing from assignee soonhokong, missing label for release notes (waiting on @RussTedrake and @soonhokong)

---

tools/workspace/ibex/package.BUILD.bazel, line 16 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Replace "notice" with "restricted" and LGPL with LGPL-3.0-only.

FYI by convention in Drake, we use the SPDX identifiers:

https://spdx.org/licenses/LGPL-3.0-only.html

Update:

Filib is LGPL-2.1-or-later so we want

licenses(["restricted])  # LGPL-2.1-or-later AND LGPL-3.0-only

---

tools/workspace/ibex/package.BUILD.bazel, line 18 at r1 (raw file):

licenses(["notice"])  # LGPL

package(default_visibility = ["//visibility:public"])

There are several rules below that are probably missing visibility = ["//visibility:private"], e.g., some of the helper genrules. It's probably easier just to set this whole package default to private here, and then just mark the things we care about as public (the final library itself, and the install target). Especially so since some of the targets risk LGPL infection if we handle them incorrectly.

---

tools/workspace/ibex/package.BUILD.bazel, line 26 at r1 (raw file):

    src = "filibsrc-3.0.2.2/rounding_control/rounding_control_config.hpp.in",
    out = "filibsrc-3.0.2.2/rounding_control/rounding_control_config.hpp",
    defines = ["define_have_sse=#define HAVE_SSE", "define_have_x87="],

Do we want to turn on SSE even on macOS? To my knowledge, nothing yet in Drake has required SSE for macOS.

---

tools/workspace/ibex/package.BUILD.bazel, line 32 at r1 (raw file):

# Note: We cannot glob the files since they are generated by the
# `extract_filib` rule.  Instead, we list them explicitly here.
FILIB_FILES = [

nit For all of these FILIB... constants, add a leading _ to the start to indicate that they should be private (unless your intent is to publish them as constants for users)?

---

tools/workspace/ibex/package.BUILD.bazel, line 187 at r1 (raw file):

    ],
    outs = FILIB_FILES,
    cmd = "tar -xzvf $(location interval_lib_wrapper/filib/3rd/filibsrc-3.0.2.2.tar.gz) -C $(@D) && (patch -p1 -d $(@D) <$(location interval_lib_wrapper/filib/3rd/filibsrc-3.0.2.2.all.all.patch) || true)",  # noqa

It's not clear to me why we want to allow the patch command to fail -- it seems like if it's failing, that should result in a build error in Drake?

---

tools/workspace/ibex/package.BUILD.bazel, line 187 at r1 (raw file):

    ],
    outs = FILIB_FILES,
    cmd = "tar -xzvf $(location interval_lib_wrapper/filib/3rd/filibsrc-3.0.2.2.tar.gz) -C $(@D) && (patch -p1 -d $(@D) <$(location interval_lib_wrapper/filib/3rd/filibsrc-3.0.2.2.all.all.patch) || true)",  # noqa

The "v" sub-command in "tar" spams the console, so please remove the v. For the same reason, we need to pass --quiet to the patch command.

---

tools/workspace/ibex/package.BUILD.bazel, line 195 at r1 (raw file):

    hdrs = FILIB_HEADER_FILES,
    includes = ["filibsrc-3.0.2.2"],
    copts = ["-w -frounding-math -ffloat-store -DHAVE_BOOST"],

I can only assume that rounding-math and float-store are very important for the correctness of the interval arithmetic. If so, we should add a cross-reference here to the upstream build system file that we're mimicking, so that we can preserve it exactly.

That will probably be easiest if copts was a list of strings, rather than a whitespace-delimited single string. (In any case, having bazel split the whitespace is dis-preferred and on its way to deprecation.)

---

tools/workspace/ibex/package.BUILD.bazel, line 201 at r1 (raw file):

# Now we actually build IBEX.

# IBEX has the following dependencies in addition to those declared in deps:

I'm not sure that this is a useful comment anymore? We aren't building those dependencies here, so their licenses are not terribly relevant?

---

tools/workspace/ibex/package.BUILD.bazel, line 209 at r1 (raw file):

# the cmake configure step.

OPERATORS_INCLUDES = """

For all of these substitution variables, are you able to provide any comment or hyperlink to say where a future Drake maintainer should start looking if they have a need to update and/or resync them with what Ibex expectects, e.g., if we're upgrading Ibex? Did you just run the ibex build by hand and inspect its intermediate results? Is there a specific sub-directory in the ibex source tree that is related to these variables?

---

tools/workspace/ibex/package.BUILD.bazel, line 252 at r1 (raw file):

    # The default INTERVAL_LIB (gaol) is GPL.  filib is lesser-GPL.
    "INTERVAL_LIB=filib",
    # TODO(russt): LP_LIB=clp

nit I'm not sure what this TODO is trying to say?

---

tools/workspace/ibex/package.BUILD.bazel, line 337 at r1 (raw file):

    ],
    copts = ["-w -std=c++11"],
    licenses = [

nit We wight as well just remove this licenses ... -- the stanza atop this BUILD file is sufficient, and keeping both copies in sync in annoying.

---

tools/workspace/ibex/package.BUILD.bazel, line 343 at r1 (raw file):

    linkopts = [
        "-lClp",
        "-lCoinUtils",

Can we just @clp in deps, instead of these two prior lines? No transcription necessary.

---

tools/workspace/ibex/package.BUILD.bazel, line 357 at r1 (raw file):

    linkstatic = True,
)

As per the example of drake/tools/workspace/lcm, Drake must link IBEX dynamically, not statically because of LGPL infection.

So, we need a bit more machinery here.

[RussTedrake]

Thanks for the fast review. I'll let Soonho answer the first few.
I've addressed all but the switch from static to dynamic linking. It causes some scary issues, and your help would definitely be appreciated there. I also had some trouble switching the SSE options for mac.

Reviewable status: 16 unresolved discussions, LGTM missing from assignee soonhokong, missing label for release notes (waiting on @ibex, @jwnimmer-tri, @RussTedrake, and @soonhokong)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Includes patches to dreal and IbexSolver from @soonhokong.

I'm not sure exactly what this means, but if any of the content of this PR (as opposed to a git remote that this PR is referring to) was written by Soonho, then please add a Co-authored-by line to the bottom of the commit message, for attribution.

Done.

---

---

tools/workspace/ibex/BUILD.bazel, line 11 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In the parent directory tools/workspace/BUILD.bazel, we need to fix _DRAKE_EXTERNAL_PACKAGE_INSTALLS.

Mostly importantly, the ibex install should not be Ubuntu-only anymore.

But also, ibex should move into the top list (next to ignition_math) so that we install from it's @ibex//:install directly, at which point we should also remove the install stanza from this BUILD file. There's no need to launder the rule through this directory anymore.

I think we need to be clear about whether we are installing ibex/ibexsolver even when when have dreal disabled. The solvers/BUILD.bazel currently only builds ibex_solver conditionally on also having dreal.

I've changed to call @ibex//:install directly, but left it in the conditional stanza. WDYT?

---

tools/workspace/ibex/package.BUILD.bazel, line 16 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Update:

Filib is LGPL-2.1-or-later so we want

licenses(["restricted])  # LGPL-2.1-or-later AND LGPL-3.0-only

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 18 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

There are several rules below that are probably missing visibility = ["//visibility:private"], e.g., some of the helper genrules. It's probably easier just to set this whole package default to private here, and then just mark the things we care about as public (the final library itself, and the install target). Especially so since some of the targets risk LGPL infection if we handle them incorrectly.

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 26 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Do we want to turn on SSE even on macOS? To my knowledge, nothing yet in Drake has required SSE for macOS.

I wasn't sure about the platform cases here. I'm sure you're right.

I tried adding a select, but had trouble (have asked on drake-dev slack bazel channel)

---

tools/workspace/ibex/package.BUILD.bazel, line 32 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit For all of these FILIB... constants, add a leading _ to the start to indicate that they should be private (unless your intent is to publish them as constants for users)?

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 187 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It's not clear to me why we want to allow the patch command to fail -- it seems like if it's failing, that should result in a build error in Drake?

Done. Good catch. That was inserted as a stopgap while we were updating the patches, and I forgot to remove it.

---

tools/workspace/ibex/package.BUILD.bazel, line 187 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The "v" sub-command in "tar" spams the console, so please remove the v. For the same reason, we need to pass --quiet to the patch command.

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 195 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can only assume that rounding-math and float-store are very important for the correctness of the interval arithmetic. If so, we should add a cross-reference here to the upstream build system file that we're mimicking, so that we can preserve it exactly.

That will probably be easiest if copts was a list of strings, rather than a whitespace-delimited single string. (In any case, having bazel split the whitespace is dis-preferred and on its way to deprecation.)

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 201 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'm not sure that this is a useful comment anymore? We aren't building those dependencies here, so their licenses are not terribly relevant?

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 209 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For all of these substitution variables, are you able to provide any comment or hyperlink to say where a future Drake maintainer should start looking if they have a need to update and/or resync them with what Ibex expectects, e.g., if we're upgrading Ibex? Did you just run the ibex build by hand and inspect its intermediate results? Is there a specific sub-directory in the ibex source tree that is related to these variables?

Done. I read the cmakefiles to convince myself it wasn't platform specific, but did just go with the generated file content. I've added that to the comment.

---

tools/workspace/ibex/package.BUILD.bazel, line 252 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I'm not sure what this TODO is trying to say?

Done. It was done (below), and I forgot to remove it. Apologies.

---

tools/workspace/ibex/package.BUILD.bazel, line 343 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Can we just @clp in deps, instead of these two prior lines? No transcription necessary.

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 357 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

As per the example of drake/tools/workspace/lcm, Drake must link IBEX dynamically, not statically because of LGPL infection.

So, we need a bit more machinery here.

I've removed linkstatic, but that has generated some scary looking errors in some of the other drake tests. I'll push this version, but this is where I'd definitely appreciate your help.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We strongly discourage building from forks (vs https://github.com/ibex-team/ibex-lib/ upstream), because it makes incorporating upstream upgrades much more difficult. We'd much rather cherry-pick this fork's patches into Drake patch files directly (and apply them with patches = [] in the repository rule).

However, since we can't upgrade past 2.8.6 anytime soon anyway, the immediate impact here is low. Furthermore, at least this github fork will be easier for us to maintain than the current system of a pre-compiled *.deb that this PR is replacing, so at least it's a strict improvement.

Therefore, how about just adding this here:

        # TODO(jwnimmer-tri) Switch to upstream ibex-lib (using local patch
        # files if necessary).

Done. The local patch file might need to include the generated files from bison and flex, too (or we'll need to add those dependencies and build rules to drake).

---

tools/workspace/ibex/repository.bzl, line 13 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Typo -- I think we need < 2.8.7 not <= 2.8.7?

Done.

[RussTedrake]

---

tools/workspace/ibex/BUILD.bazel, line 11 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I think we need to be clear about whether we are installing ibex/ibexsolver even when when have dreal disabled. The solvers/BUILD.bazel currently only builds ibex_solver conditionally on also having dreal.

I've changed to call @ibex//:install directly, but left it in the conditional stanza. WDYT?

It seems by not writing the quotes around @ibex//:install, I've summoned user ibex to this PR. my apologies.

[jwnimmer-tri]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee soonhokong, missing label for release notes (waiting on @jwnimmer-tri, @RussTedrake, and @soonhokong)

---

-- commits, line 10 at r2:
nit I think we need <> around the email address in order for GitHub to parse this correctly. When parsed correctly, the commit will show up with two headshots on the GitHub PR page.

---

tools/workspace/ibex/BUILD.bazel, line 11 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

It seems by not writing the quotes around @ibex//:install, I've summoned user ibex to this PR. my apologies.

OK You're right, the install should be conditional on solvers selection. It's correct now.

---

tools/workspace/ibex/package.BUILD.bazel, line 26 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I wasn't sure about the platform cases here. I'm sure you're right.

I tried adding a select, but had trouble (have asked on drake-dev slack bazel channel)

Working

I'll see what I can discover.

Another option would be to eschew SSE for all platforms, at least for now.

---

tools/workspace/ibex/package.BUILD.bazel, line 195 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done.

OK Ah, well, from the comment it sounds like it doesn't matter at all for x86_64 or arm64, but we might as well match upstream.

---

tools/workspace/ibex/package.BUILD.bazel, line 357 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I've removed linkstatic, but that has generated some scary looking errors in some of the other drake tests. I'll push this version, but this is where I'd definitely appreciate your help.

Working

I'll develop a patch a bit later on. (We actually always want linkstatic=True, so I'll be putting that back. We just need more machinery beyond that.)

[RussTedrake]

@drake-jenkins-bot mac-catalina-clang-bazel-continuous-release please (curious what happens now with the SSE left in)

[RussTedrake]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee soonhokong, missing label for release notes (waiting on @jwnimmer-tri, @RussTedrake, and @soonhokong)

---

-- commits, line 10 at r2:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I think we need <> around the email address in order for GitHub to parse this correctly. When parsed correctly, the commit will show up with two headshots on the GitHub PR page.

Done. TIL.

---

tools/workspace/ibex/package.BUILD.bazel, line 357 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I'll develop a patch a bit later on. (We actually always want linkstatic=True, so I'll be putting that back. We just need more machinery beyond that.)

Thanks. Just to be clear, we'll wait for that patch before this lands? (I'm not trying to rush, just trying to make sure I understand your intention).

I've restored the linkstatic here since I had to push anyhow.

[RussTedrake]

@drake-jenkins-bot mac-catalina-clang-bazel-continuous-release please

[RussTedrake]

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please

[jwnimmer-tri]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee soonhokong, missing label for release notes (waiting on @jwnimmer-tri, @RussTedrake, and @soonhokong)

a discussion (no related file):
Working

Compared to the Drake v0.35.0 release, this PR removes these two files from the install image:

v0.35.0/drake/lib/ibex/3rd/libprim.la
v0.35.0/drake/lib/ibex/3rd/libprim.a

Are those files that are supposed to be installed @soonhokong? My first reaction is that we are OK to remove them.

---

---

tools/workspace/ibex/package.BUILD.bazel, line 357 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Thanks. Just to be clear, we'll wait for that patch before this lands? (I'm not trying to rush, just trying to make sure I understand your intention).

I've restored the linkstatic here since I had to push anyhow.

RussTedrake#44

Yes, we must comply with LGPL prior to this hitting master (so that we can continue to redistribute SNOPT in our nightly binaries).

[soonhokong]

Reviewable status: 6 unresolved discussions, LGTM missing from assignee soonhokong, missing label for release notes (waiting on @jwnimmer-tri and @RussTedrake)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Compared to the Drake v0.35.0 release, this PR removes these two files from the install image:

v0.35.0/drake/lib/ibex/3rd/libprim.la
v0.35.0/drake/lib/ibex/3rd/libprim.a

Are those files that are supposed to be installed @soonhokong? My first reaction is that we are OK to remove them.

FYI, this is part of filib. I'm not sure if this is actually used in ibex (and transitively in dReal) though.

---

---

solvers/test/ibex_solver_test.cc, line 111 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Why is there a , {} here now? It's (still) just the default.

I agree that we can remove this part.

@RussTedrake , could you update this?

---

solvers/test/ibex_solver_test.cc, line 117 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Should we just use M_SQRT_2 here instead?

Yes.

@RussTedrake , could you update this?

---

tools/workspace/dreal/ibex_2.8.6.patch, line 34 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit To my eye, Drake is not ever building dReal's unit tests, so any patches like this to a unit test would be irrelevant?

Or, if we do need to keep it for some reason, then to comment out code via a patch file always use #if 0 (not //) for improved conflict resolution and re-basing.

Yes, we can remove the changes on box_test.cc.

@RussTedrake , could you update this?

[jwnimmer-tri]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee soonhokong, missing label for release notes (waiting on @jwnimmer-tri and @RussTedrake)

a discussion (no related file):

Previously, soonhokong (Soonho Kong) wrote…

FYI, this is part of filib. I'm not sure if this is actually used in ibex (and transitively in dReal) though.

OK should be fine to omit, I suppose. Probably was just an accident it was there originally.

---

[jwnimmer-tri]

Reviewed 1 of 2 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee soonhokong, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake)

---

tools/workspace/ibex/package.BUILD.bazel, line 26 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I'll see what I can discover.

Another option would be to eschew SSE for all platforms, at least for now.

OK now (via TODO).

[RussTedrake]

I botched the commits a moment ago (git a commit --amend too quickly), but have fixed them now.

Reviewable status: LGTM missing from assignee soonhokong, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)

---

solvers/test/ibex_solver_test.cc, line 111 at r1 (raw file):

Previously, soonhokong (Soonho Kong) wrote…

I agree that we can remove this part.

@RussTedrake , could you update this?

Done.

---

solvers/test/ibex_solver_test.cc, line 117 at r1 (raw file):

Previously, soonhokong (Soonho Kong) wrote…

Yes.

@RussTedrake , could you update this?

Done.

---

tools/workspace/dreal/ibex_2.8.6.patch, line 34 at r1 (raw file):

Previously, soonhokong (Soonho Kong) wrote…

Yes, we can remove the changes on box_test.cc.

@RussTedrake , could you update this?

Done.

---

tools/workspace/ibex/package.BUILD.bazel, line 26 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I'll see what I can discover.

Another option would be to eschew SSE for all platforms, at least for now.

I've disabled it and added a TODO. Seems like it shouldn't be hard to get right; I was surprised the easy thing didn't work.

---

tools/workspace/ibex/package.BUILD.bazel, line 357 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

RussTedrake#44

Yes, we must comply with LGPL prior to this hitting master (so that we can continue to redistribute SNOPT in our nightly binaries).

Done. Grabbed it. Thank you.

[RussTedrake]

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please

[jwnimmer-tri]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: LGTM missing from assignee soonhokong, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake)

[RussTedrake]

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please

[jwnimmer-tri]

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: LGTM missing from assignee soonhokong, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake)

[soonhokong]

Reviewed 4 of 10 files at r1, 3 of 5 files at r2, 2 of 3 files at r5, 1 of 2 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake)

[soonhokong]

@RussTedrake , I was investigating the test failures on macOS. I've found that re-enabling define_have_sse in tools/workspace/ibex/package.BUILD.bazel makes those failed tests pass.

-    defines = ["define_have_sse=", "define_have_x87="],
+    defines = [
+        "define_have_sse=#define HAVE_SSE",
+        "define_have_x87=",
+    ],

I still need more time to understand why it is the case. Also note that we do not reproduce the errors on Ubuntu.

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake)

[jwnimmer-tri]

If the answer ends up being to require SSE on macOS when using dReal, then we can document that fact. (Especially if our IBEX brew bottles were already using SSE.)

[soonhokong]

Especially if our IBEX brew bottles were already using SSE

This is the case. Unless we pass --disable-sse2, it tries to use -msse3 and use -msse2 if -msse3 fails. We have not passed --disable-sse2.

https://stackoverflow.com/questions/45917280/what-is-the-minimum-supported-sse-flag-that-can-be-enabled-on-macos#comment78801661_45921250

SSSE3 is the correct baseline for a 64-bit processes targeting an arbitrary macOS release. If targeting macOS 10.12 or later, you can assume SSE4.1 as well, since 10.11.x was the last OS version to support pre-Penryn hardware.

Given Drake's supported configuration matrix (10.15 and 11), I think it's safe to enable SSE.

[jwnimmer-tri]

SGTM.

[RussTedrake]

I will try (probably tonight). But I believe mac CI failed the first time when we had SSE enabled.

[soonhokong]

@BetsyMcPhail , do you know the model of mac minis that Drake CI is using via MacStadium? I remember that there was an attempt to upgrade it from the 2014 macmini model to the 2018 model?

[BetsyMcPhail]

It looks like we are using Macmini8,1

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake)

[soonhokong]

It looks like we are using Macmini8,1

So, it's using https://ark.intel.com/content/www/us/en/ark/products/126688/intel-core-i38100-processor-6m-cache-3-60-ghz.html or a similar one. FYI, it supports SSE4.1, SSE4.2, and AVX2.

[jwnimmer-tri]

I pushed a patch to use SSE unconditionally. Let's see what happens!

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-everything-release please

[jwnimmer-tri]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake)

[soonhokong]

Let's see what happens!

CI is all green!

[jwnimmer-tri]

It seems to me like this PR is sufficient, so I've merged it in. We'll get the full weekend CI suite (even more configs that the everyday nightlies) to check it.