[workspace] Build NLopt from source #17288
Conversation
jwnimmer-tri
added
priority: low
release notes: newly deprecated
|
@drake-jenkins-bot linux-focal-clang-bazel-experimental-address-sanitizer please |
There was a problem hiding this comment.
+(status: do not review)
Reviewable status: 5 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)
-- commits line 4 at r1:
Working
Check homebrew and Ubuntu sources to see if they've added any useful patching which we'd now be losing out on.
tools/workspace/dreal/repository.bzl line 18 at r1 (raw file):
], repo_mapping = { "@nlopt": "@nlopt_internal",
Working
Since we don't do dReal counterexamples in Drake, it seems like we should be able to stop using nlopt, but I haven't figured out how so far.
tools/workspace/nlopt_internal/package.BUILD.bazel line 50 at r1 (raw file):
outs = ["src/api/nlopt.hpp"], cmd = " ".join([ "cmake",
Working
Possibly we should vendor the enums into out git, so that we can avoid using CMake at build-time. (We would still use it a test-time to check that our enums are up-to-date.)
tools/workspace/nlopt_internal/package.BUILD.bazel line 65 at r1 (raw file):
":nlopt_config.h", # This list exactly matches CMakeLists.txt NLOPT_SOURCES, except that # we have removed all of the "src/algs/luksan/**" sources (LGPL).
Working
Possibly we should have a cross-check on the list of sources vs CMakeLists.
tools/workspace/nlopt_internal/patches/remove-luksan.patch line 1 at r1 (raw file):
TBD
Working
Describe what's going on here.
|
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please |
jwnimmer-tri
force-pushed
the
nlopt-internal
branch
from
9252809
to
520cf77
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Check homebrew and Ubuntu sources to see if they've added any useful patching which we'd now be losing out on.
OK
Hombrew doesn't add patches, it just disables multi-language bindings.
Ubuntu has patches to fix the install locations and soname, but nothing to the code itself.
tools/workspace/dreal/repository.bzl line 18 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Since we don't do dReal counterexamples in Drake, it seems like we should be able to stop using nlopt, but I haven't figured out how so far.
OK actually we do seem to have use_local_optimization set to true by default, so this will need to stay.
Lines 566 to 570 in f8d36f4
tools/workspace/nlopt_internal/package.BUILD.bazel line 50 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Possibly we should vendor the enums into out git, so that we can avoid using CMake at build-time. (We would still use it a test-time to check that our enums are up-to-date.)
Done.
tools/workspace/nlopt_internal/package.BUILD.bazel line 58 at r1 (raw file):
) # TODO(jwnimmer-tri) Compile this using vendor_cxx.
Working
To avoid loader conflicts, we should do this immediately as part of this PR.
tools/workspace/nlopt_internal/package.BUILD.bazel line 65 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Possibly we should have a cross-check on the list of sources vs CMakeLists.
Done.
tools/workspace/nlopt_internal/patches/remove-luksan.patch line 1 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Describe what's going on here.
Done.
jwnimmer-tri
force-pushed
the
nlopt-internal
branch
from
520cf77
to
b5ae6e8
Compare
There was a problem hiding this comment.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
tools/workspace/nlopt_internal/package.BUILD.bazel line 58 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
To avoid loader conflicts, we should do this immediately as part of this PR.
Done.
jwnimmer-tri
force-pushed
the
nlopt-internal
branch
from
b5ae6e8
to
85151ef
Compare
|
@drake-jenkins-bot linux-focal-clang-bazel-experimental-address-sanitizer please |
jwnimmer-tri
added
release notes: fix
There was a problem hiding this comment.
+@rpoyner-tri for feature review, please.
(Apologies; there's a few typos I missed originally, noted in review comments. I'll push a fix for them after the bonus CI runs return their results.)
\CC @hongkai-dai FYI
Reviewable status: 4 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @rpoyner-tri)
tools/workspace/dreal/patches/warnings.patch line 1 at r2 (raw file):
TBD
Working
The problem we're dealing with here is this console spam:
external/dreal/dreal/optimization/nlopt_optimizer.h:44:7: warning: 'dreal::NloptOptimizer' declared with greater visibility than the type of its field 'dreal::NloptOptimizer::opt_' [-Wattributes]
I need to place a comment here (instead of the TBD) explaining that, and probably a TODO here to try to vendor-build dReal as well.
tools/workspace/nlopt_internal/package.BUILD.bazel line 191 at r2 (raw file):
) # The _SRCS_AGS and _HDRS_AGS cover the NLOPT_CXX sources from the upstream
Working
typo
Suggestion:
NLOPT_CXX11
tools/workspace/nlopt_internal/test/enum_test.py line 41 at r2 (raw file):
expected = re.sub(r'\s+\\', r' \\', expected) # Compare
Working
nit punc
tools/workspace/nlopt_internal/patches/remove_luksan.patch line 1 at r2 (raw file):
Remove NLopt's dependency on it's internal luksan algorithm library.
Working
typo
Suggestion:
its
|
a discussion (no related file): macOS build indicates that Ubuntu is probably still leaking system NLopt a bit. |
jwnimmer-tri
force-pushed
the
nlopt-internal
branch
2 times, most recently
from
aacd8f1
to
5fa3927
Compare
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
macOS build indicates that Ubuntu is probably still leaking system NLopt a bit.
Done.
|
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please |
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
|
+@sammy-tri for platform review, if you have time please? You have some familiarity with the solvers externals' build system over the years. (Sorry, I meant to tag @ggould-tri on call today but the updated Reviewable dashboard makes those reminders more difficult to recall now.) |
There was a problem hiding this comment.
@sammy-tri btw, if you don't have the time, I'd be happy to take this platform review for you.
Reviewed 4 of 11 files at r1, 6 of 15 files at r2, 1 of 4 files at r3, all commit messages.
Reviewable status: LGTM missing from assignees rpoyner-tri(platform),sammy-tri(platform) (waiting on @rpoyner-tri and @sammy-tri)
|
FYI the rebase surfaces a build error. I'll need to make some small edits here before pushing it. |
jwnimmer-tri
force-pushed
the
nlopt-internal
branch
from
5fa3927
to
c71afed
Compare
There was a problem hiding this comment.
Reviewed 4 of 11 files at r1, 10 of 15 files at r2, 3 of 4 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform) (waiting on @jwnimmer-tri and @sammy-tri)
tools/workspace/nlopt_internal/patches/gen_enums.patch line 4 at r5 (raw file):
Here we create the same effect by patching the source file directly. Separately, we cross-check that this patch the CMake script produce
grammar? missing words?
Code quote:
this patch the CMake script producetools/workspace/nlopt_internal/test/enum_test.py line 14 at r5 (raw file):
def test_enum_cross_check(self): """Checks that the Drake-created flavor of nlopt.cpp (via a patch file) is consistent with the upstream-geneated flavor of same (via CMake).
typo
Code quote:
geneatedtools/workspace/nlopt_internal/test/enum_test.py line 21 at r5 (raw file):
# Load both input files. # "actual" refers to the the Drake-created flavor (via a patch file). # "expected" refers to the upstream-geneated flavor (via CMake).
typo
Code quote:
geneatedtools/workspace/nlopt_internal/patches/remove_luksan.patch line 3 at r5 (raw file):
Remove NLopt's dependency on its internal luksan algorithm library. That library is licensenced under LGPL-2.1+ but the rest of NLopt is
typo
Code quote:
licensenced
There was a problem hiding this comment.
Let's do +@ggould-tri -@sammy-tri for platform review load balancing.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on @rpoyner-tri)
tools/workspace/nlopt_internal/patches/gen_enums.patch line 4 at r5 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
grammar? missing words?
Done.
jwnimmer-tri
force-pushed
the
nlopt-internal
branch
from
c71afed
to
13b33a9
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)
|
a discussion (no related file): |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)
a discussion (no related file):
Previously, ggould-tri wrote…
Running this locally, I get bazel errors even with a clean--expunge. Not sure if this is an actual bazel issue or not.
ggould@baymax:~/git/drake$ bazel clean --expunge INFO: Invocation ID: 275f3581-043e-4f60-ab7c-5522fb5ea81c INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes. ggould@baymax:~/git/drake$ bazel test @nlopt_internal//... Starting local Bazel server and connecting to it... INFO: Invocation ID: 742f4597-1e37-463d-9191-3eae020dab63 INFO: Analyzed 10 targets (49 packages loaded, 471 targets configured). INFO: Found 10 targets and 0 test targets... ERROR: /media/sdb1/ggould/.cache/bazel/_bazel_ggould/330cdd996cb32c1347931fe9e349adad/external/nlopt_internal/BUILD.bazel:331:8: Middleman _middlemen/external_Snlopt_Uinternal_Sinstall-runfiles failed: missing input file 'external/nlopt_internal/src/AUTHORS', owner: '@nlopt_internal//:src/AUTHORS' ERROR: /media/sdb1/ggould/.cache/bazel/_bazel_ggould/330cdd996cb32c1347931fe9e349adad/external/nlopt_internal/BUILD.bazel:331:8: Middleman _middlemen/external_Snlopt_Uinternal_Sinstall-runfiles failed: missing input file 'external/nlopt_internal/src/NEWS', owner: '@nlopt_internal//:src/NEWS' ERROR: /media/sdb1/ggould/.cache/bazel/_bazel_ggould/330cdd996cb32c1347931fe9e349adad/external/nlopt_internal/BUILD.bazel:331:8: Middleman _middlemen/external_Snlopt_Uinternal_Sinstall-runfiles failed: 2 input file(s) do not exist ERROR: /media/sdb1/ggould/.cache/bazel/_bazel_ggould/330cdd996cb32c1347931fe9e349adad/external/nlopt_internal/BUILD.bazel:331:8 Middleman _middlemen/external_Snlopt_Uinternal_Sinstall-runfiles failed: 2 input file(s) do not exist INFO: Elapsed time: 8.340s, Critical Path: 0.08s INFO: 21 processes: 21 internal. FAILED: Build did NOT complete successfully FAILED: Build did NOT complete successfully
Aha, this PR fails to install the license notices. Working.
Compared to the Ubuntu and macOS packages, this removes the LGPL'd "luksan" solver from the build, which means we can link the library statically (and eventually, hidden). This might mean that NLopt is no longer able to solve certain kinds of programs, but that is a fair trade-off given all of the problems caused by LGPL linking. The package.BUILD.bazel is modeled on nlopt.BUILD.bazel in a10ebb7. Deprecate the now-unused current (host OS) nlopt external.
jwnimmer-tri
force-pushed
the
nlopt-internal
branch
from
13b33a9
to
3de9fd6
Compare
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Aha, this PR fails to install the license notices. Working.
Done.
There was a problem hiding this comment.
Reviewed 3 of 15 files at r2, 1 of 1 files at r4, 3 of 3 files at r5, 3 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform) (waiting on @jwnimmer-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done.
Verified working.
Compared to the Ubuntu and macOS packages, this removes the LGPL'd "luksan" solver from the build, which means we can link the library statically (and eventually, hidden). This might mean that NLopt is no longer able to solve certain kinds of programs, but that is a fair trade-off given all of the problems caused by LGPL linking.
The
package.BUILD.bazelis modeled onnlopt.BUILD.bazelin a10ebb7.Deprecate the now-unused current (host OS) nlopt external.
Towards #17231 and #16872.
This change is