[workspace] Build vendored clp_internal from source #19928
Conversation
jwnimmer-tri
added
priority: low
status: do not merge
status: do not review
release notes: newly deprecated
jwnimmer-tri
force-pushed
the
workspace-clp-source-rebuild
branch
from
379a0d1 to
514d796
Compare
jwnimmer-tri
force-pushed
the
workspace-clp-source-rebuild
branch
from
514d796 to
dcecc30
Compare
jwnimmer-tri
force-pushed
the
workspace-clp-source-rebuild
branch
from
dcecc30 to
fdde418
Compare
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-release please |
jwnimmer-tri
force-pushed
the
workspace-clp-source-rebuild
branch
from
fdde418 to
2df255f
Compare
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-release please |
|
+@rpoyner-tri for feature review, please. |
jwnimmer-tri
force-pushed
the
workspace-clp-source-rebuild
branch
from
2df255f to
d2d3d79
Compare
There was a problem hiding this comment.
Reviewed 15 of 24 files at r1, 9 of 10 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers
There was a problem hiding this comment.
Reviewed 15 of 24 files at r1, 6 of 10 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers
tools/workspace/coinutils_internal/defs.bzl line 182 at r4 (raw file):
outs = [vendor_src + ".patch"], cmd = " ".join([ "(diff -U0",
btw I'm curious what difference this makes.
tools/workspace/ipopt_internal_fromsource/package.BUILD.bazel line 383 at r4 (raw file):
], allowed_externals = [ "@drake//tools/workspace/ipopt_internal_fromsource:patches",
The changes in this file look like strays from other work, maybe?
Deprecate the clp external. Having discovered during Clp that we don't actually have <ieeefp.h> available on any of our platforms, fix that same mistake in Ipopt build rules. It's currently dead code there, but would be a time bomb on upgrades in case it ever did start to be used.
jwnimmer-tri
force-pushed
the
workspace-clp-source-rebuild
branch
from
d2d3d79 to
2722a17
Compare
There was a problem hiding this comment.
+@sherm1 for platform review per schedule, please.
Reviewed all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sherm1(platform)
tools/workspace/coinutils_internal/defs.bzl line 182 at r4 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
btw I'm curious what difference this makes.
macOS rejects the -u spelling as deprecated.
tools/workspace/ipopt_internal_fromsource/package.BUILD.bazel line 383 at r4 (raw file):
This hunk is not stray:
The glob might be empty if we don't have any patches at the moment,
but we'll keep the boilerplate here as a fail-safe.
I'll edit the comment message to mention the AUTOCONF_UNDEFINES.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sherm1(platform)
tools/workspace/coinutils_internal/defs.bzl line 182 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
macOS rejects the
-uspelling as deprecated.
(Which wasn't a problem for Ipopt because we only build that from source on Ubuntu. Now that we're building more Coin stuff from source for both platforms, it became relevant.)
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sherm1(platform)
tools/workspace/ipopt_internal_fromsource/package.BUILD.bazel line 383 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This hunk is not stray:
The glob might be empty if we don't have any patches at the moment,
but we'll keep the boilerplate here as a fail-safe.
I'll edit the comment message to mention the AUTOCONF_UNDEFINES.
Wow. That was supposed to be a quoted # comment, not NEW YORK CASE.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)
There was a problem hiding this comment.
There's a lot of repetition and magical boilerplate here which makes me wonder if future upgrades will be difficult to do for the COIN packages. But ... I don't know of a better way. Platform
Reviewed 15 of 24 files at r1, 6 of 10 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sherm1(platform)
Towards #17231.
This change is