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

python3: Support Python2 for lcm when being used by drake_visualizer #9699

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Oct 18, 2018

For #8352
Towards #9614

Allows us to use Python2 builds of director when building other stuff with Python3.

Install for Python2+3, using this setup script:

$ bazel run @lcm//:install -- ~+/build/install2
...
-- Installing: /home/eacousineau/proj/tri/repo/branches/drake/py3/drake/build/install2/lib/python2.7/site-packages/lcm/_lcm.so
-- Up-to-date: /home/eacousineau/proj/tri/repo/branches/drake/py3/drake/build/install2/lib/python2.7/site-packages/lcm/__init__.py
...
$ bazel-py3 run @lcm//:install -- ~+/build/install3
...
-- Installing: /home/eacousineau/proj/tri/repo/branches/drake/py3/drake/build/install3/lib/python3.5/site-packages/lcm/_lcm.cpython-35m-x86_64-linux-gnu.so
-- Up-to-date: /home/eacousineau/proj/tri/repo/branches/drake/py3/drake/build/install3/lib/python3.5/site-packages/lcm/__init__.py
-- Installing: /home/eacousineau/proj/tri/repo/branches/drake/py3/drake/build/install3/lib/python2.7/site-packages/lcm/_lcm.so
-- Up-to-date: /home/eacousineau/proj/tri/repo/branches/drake/py3/drake/build/install3/lib/python2.7/site-packages/lcm/__init__.py
...

Admittedly, the py2_py3 macro may be a little too cute for now. If we think it's OK to just always compile LCM in Python2 + Python3, I could just manually expand those targets.


This change is Reviewable

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

+@jamiesnape for review when you have a chance, please.

Let me know if there's a better packaging solution than this.

Reviewable status: all discussions resolved, platform LGTM missing

@EricCousineau-TRI EricCousineau-TRI mentioned this pull request Oct 18, 2018
21 tasks
@EricCousineau-TRI
Copy link
Contributor Author

Need to fix ctypes.cdll shenanigans...

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Fixed ctypes.cdll issue, but CI is showing stale results, possibly due to GitHub outages.

Reviewable status: all discussions resolved, LGTM missing from assignee jamiesnape, platform LGTM missing

@EricCousineau-TRI
Copy link
Contributor Author

Re-adding "do not merge"; forgot that some of the code has "DO NOT MERGE" bits to exemplify what config could look like.

@jwnimmer-tri
Copy link
Collaborator

Would it be crazy to ship our pre-built drake-visualizer images with a private lcm-python that was already compiled? I'm not sure that we care about liblcm runtime compatibility between drake-visualizer and other python-based code within drake. If the visualizer is the only thing that needs python2 always, it might be a nice win to remove that use case from Drake's build, instead of adding all of this complexity.

@jamiesnape
Copy link
Contributor

Not crazy at all if we keep LCM reasonably in sync (keeping on a tag would be nice).

@jamiesnape
Copy link
Contributor

I think we only really care about wire compatibility in this case anyway, so the definition of “reasonably in sync” is loose.

@EricCousineau-TRI
Copy link
Contributor Author

Aye, that'd work. How do we want to formulate that objective? Should I make a separate issue, or should we just track it under #8352?

Will close this PR once we do that.

@jamiesnape
Copy link
Contributor

Create a separate issue and assign it to me. I assume the Python sources for the lcmtypes are 2/3 compatible?

@EricCousineau-TRI
Copy link
Contributor Author

Seems that way; shall do!

@EricCousineau-TRI
Copy link
Contributor Author

Shall be superseded by the solution to #9764

@jamiesnape jamiesnape removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants