Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[workspace] Switch conex to use hidden namespace #19892

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jul 31, 2023

Towards #17231.

See also ToyotaResearchInstitute/conex#6.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: fix This pull request contains fixes (no new features) labels Jul 31, 2023
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review August 1, 2023 00:15
@jwnimmer-tri
Copy link
Collaborator Author

+@xuchenhan-tri for feature review, please.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers


multibody/contact_solvers/conex_supernodal_solver.cc line 7 at r1 (raw file):

#include "drake/common/drake_export.h"
#include "drake_vendor/conex/clique_ordering.h"

Working

I might try not adding the drake_vendor prefix to see if it works better.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers


multibody/contact_solvers/conex_supernodal_solver.cc line 7 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I might try not adding the drake_vendor prefix to see if it works better.

Done

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature :lgtm:
+@rpoyner-tri for platform review please.

Reviewed 5 of 8 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)


tools/workspace/BUILD.bazel line 49 at r2 (raw file):

    srcs = ["vendor_cxx.py"],
    visibility = [
        # These should all be of the form "@foo_internal//:__subpackages__".

BTW,


tools/workspace/conex_internal/vendor.bzl line 16 at r2 (raw file):

    For "supernodal_solver", declares a cc_library in the usual sense, except
    rewrites the source code so that it uses a `drake_vendor` inline hidden
    namespace wrapper so that the library's linker symbols are private.

BTW, understanding check, the namespace wrapper is applied by vendor_cxx and this bazel function simply routes to cc_library_vendored?

Code quote:

    rewrites the source code so that it uses a `drake_vendor` inline hidden
    namespace wrapper so that the library's linker symbols are private.

tools/workspace/conex_internal/vendor.bzl line 49 at r2 (raw file):

    ]
    edit_include = None
    strip_include_prefix = "/conex/drake_vendor"

BTW, I don't understand how this works. Don't the vendored sources and headers have prefix drake_vendor/conex?

Code quote:

strip_include_prefix = "/conex/drake_vendor"

tools/workspace/conex_internal/patches/eigen_namespace_pollution.patch line 1 at r2 (raw file):

[conex] Fix RLDLT to live in the conex namespace

BTW, is there a path towards merging the PR upstream? Do we really need the patch?

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)


tools/workspace/BUILD.bazel line 49 at r2 (raw file):

Previously, xuchenhan-tri wrote…

BTW,

Done?


tools/workspace/conex_internal/vendor.bzl line 16 at r2 (raw file):

Previously, xuchenhan-tri wrote…

BTW, understanding check, the namespace wrapper is applied by vendor_cxx and this bazel function simply routes to cc_library_vendored?

Yes. I've amended the comment a bit, too.


tools/workspace/conex_internal/vendor.bzl line 49 at r2 (raw file):

Previously, xuchenhan-tri wrote…

BTW, I don't understand how this works. Don't the vendored sources and headers have prefix drake_vendor/conex?

The semantics of this string differ depending on whether or not it opens with a /. It will probably be clearest, though, to use the spelling without the slash.


tools/workspace/conex_internal/patches/eigen_namespace_pollution.patch line 1 at r2 (raw file):

Do we really need the patch?

Yes. Otherwise we end up vendoring the class as drake_vendor::Eigen::RLDLT which doesn't work at all. (It can't find the real Eigen symbols.)

Is there a path towards merging the PR upstream?

See the link below on line 7. I've submitted this patch upstream as a pull request.

From past experience, though, upstream is dead. There are trivial open PRs there that are 6+ months old with no reply.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform)


tools/workspace/BUILD.bazel line 49 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done?

I was able to figure this one out but forgot to deleted the half finished thought. Apologies.


tools/workspace/conex_internal/vendor.bzl line 49 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The semantics of this string differ depending on whether or not it opens with a /. It will probably be clearest, though, to use the spelling without the slash.

Oh I see. Thanks for the reference.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 8 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),xuchenhan-tri(platform)

@rpoyner-tri rpoyner-tri merged commit 21e55be into RobotLocomotion:master Aug 1, 2023
@jwnimmer-tri jwnimmer-tri deleted the vendor-conex branch August 2, 2023 00:01
@ggould-tri
Copy link
Contributor

FYI, this PR is the primary suspect in two CI failures,

At @svenevs request we (#buildcop) are holding off on reverting this in hope of a fix-forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants