Skip to content

Commit

Permalink
[build] Fully retire the non-lcmtypes drake module
Browse files Browse the repository at this point in the history
In the past, we allowed 'import drake...' to refer to Python libraries
inside the Drake source tree. With bzlmod this is no longer practical.
Prior commits already removed most uses; this commit finishes the job.
From now on, imports should say e.g., 'from common...', instead.

Note that lcmtypes like 'from drake import lcmt_geometry_data' still
retain the 'drake' package name.
  • Loading branch information
jwnimmer-tri committed Dec 4, 2024
1 parent f4c19e7 commit 823f4aa
Show file tree
Hide file tree
Showing 10 changed files with 19 additions and 48 deletions.
11 changes: 6 additions & 5 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

load("//tools/install:install.bzl", "install", "install_test")
load("//tools/lint:lint.bzl", "add_lint_tests")
load("//tools/skylark:py.bzl", "py_library")
load("//tools/skylark:drake_py.bzl", "drake_py_library")

package(
default_visibility = ["//visibility:public"],
Expand All @@ -17,12 +17,13 @@ exports_files([
"package.xml",
])

# Drake's top-level module; all drake_py_stuff rules add this to deps.
# (We use py_library here because drake_py_library would be circular.)
# This file should NOT be installed (see commits in __init__.py).
py_library(
# A legacy hack module to disambiguate the 'drake' module when Drake is being
# used as a non-bzlmod external. We should remove this when we drop support for
# WORKSPACE (i.e., Bazel >= 9).
drake_py_library(
name = "module_py",
srcs = ["__init__.py"],
visibility = ["//lcmtypes:__pkg__"],
)

# Expose shared library for (a) installed binaries, (b) Drake Python bindings,
Expand Down
16 changes: 6 additions & 10 deletions __init__.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
# It confusing to have both drake-the-workspace and drake-the-lcmtypes-package
# on sys.path at the same time via Bazel's py_library(imports = ...).
# It brittle to have both drake-the-workspace and drake-the-lcmtypes-package
# available on the sys.path at the same time in Bazel.
#
# To prevent that confusion, and possibly also import errors, in our
# //lcmtypes:lcmtypes_drake_py rule we use add_current_package_to_imports =
# False, and then here in drake-the-workspace's package initialization we use
# __path__ editing to fold the two directories into the same package.
# To avoid import errors based on which one came first on the path, in our
# //lcmtypes:lcmtypes_drake_py rule we depend on this file, and here (in
# drake-the-workspace's package initialization) we use __path__ editing to
# fold the two directories into the same package.
#
# We need to do it on a best-effort basis, because not all of our py_binary
# rules use lcmtypes -- sometimes the lcmtypes will be absent from runfiles.
#
# Note that this file should NOT be installed (`//:install` should not touch
# it). The `//lcmtypes`-supplied init file is the correct file to install.

try:
import drake.lcmtypes
__path__.append(list(drake.lcmtypes.__path__)[0] + "/drake")
Expand Down
3 changes: 2 additions & 1 deletion bindings/pydrake/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ exports_files([
PACKAGE_INFO = get_pybind_package_info(base_package = "//bindings")

# This target provides the following Python modules:
# - drake (i.e., our lcmtypes)
# - pydrake
# - pydrake.autodiffutils
# - pydrake.common (and all of its sub-modules)
Expand All @@ -48,6 +49,7 @@ drake_py_library(
],
deps = [
"//bindings/pydrake/common:_init_py",
"//lcmtypes:lcmtypes_drake_py",
],
)

Expand Down Expand Up @@ -349,7 +351,6 @@ drake_py_unittest(
name = "lcm_test",
deps = [
":lcm_py",
"//lcmtypes:lcmtypes_drake_py",
],
)

Expand Down
1 change: 0 additions & 1 deletion bindings/pydrake/systems/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ drake_pybind_library(
":framework_py",
":module_py",
"//bindings/pydrake:lcm_py",
"//lcmtypes:lcmtypes_drake_py",
],
py_srcs = ["_lcm_extra.py"],
)
Expand Down
1 change: 0 additions & 1 deletion bindings/pydrake/visualization/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ drake_pybind_library(
"//bindings/pydrake/systems",
"//bindings/pydrake/geometry",
"//bindings/pydrake:lcm_py",
"//lcmtypes:lcmtypes_drake_py",
],
py_srcs = [
"meldis.py",
Expand Down
2 changes: 1 addition & 1 deletion common/proto/call_python_client_notebook.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"outputs": [],
"source": [
"# Example Python client.\n",
"from drake.common.proto.call_python_client import CallPythonClient"
"from common.proto.call_python_client import CallPythonClient"
]
},
{
Expand Down
17 changes: 3 additions & 14 deletions lcmtypes/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -254,23 +254,12 @@ drake_lcm_cc_library(
],
)

# Generate the *.py sources, but keep __init__.py separate, so that it doesn't
# get installed. The install will use this (private) library. Users should
# depend on ":lcmtypes_drake_py", immediately below.
drake_lcm_py_library(
name = "_generated_lcmtypes_drake_py",
# We'll rely on the //:module_py path munging instead.
add_current_package_to_imports = False,
name = "lcmtypes_drake_py",
lcm_package = "drake",
lcm_srcs = ALL_LCM_SRCS,
visibility = ["//visibility:private"],
)

drake_py_library(
name = "lcmtypes_drake_py",
srcs = ["__init__.py"],
deps = [
":_generated_lcmtypes_drake_py",
"//:module_py",
],
)

Expand Down Expand Up @@ -351,7 +340,7 @@ install(
],
}),
targets = [
":_generated_lcmtypes_drake_py",
":lcmtypes_drake_py",
] + select({
"@lcm//:lcm_install_java_is_off": [],
"//conditions:default": [
Expand Down
1 change: 0 additions & 1 deletion lcmtypes/__init__.py

This file was deleted.

2 changes: 1 addition & 1 deletion multibody/parsing/test/package_downloader_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import unittest
from urllib.error import HTTPError

import drake.multibody.parsing.package_downloader as mut
import multibody.parsing.package_downloader as mut

# We'll mock out the internet-touching methods. We don't want our unit test
# touching the internet. See setUp and tearDown below for details.
Expand Down
13 changes: 0 additions & 13 deletions tools/skylark/drake_py.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,10 @@ load("//tools/skylark:py.bzl", "py_binary", "py_library", "py_test")

def drake_py_library(
name,
deps = None,
**kwargs):
"""A wrapper to insert Drake-specific customizations."""

# Work around https://github.com/bazelbuild/bazel/issues/1567.
deps = (deps or []) + ["//:module_py"]
py_library(
name = name,
deps = deps,
srcs_version = "PY3",
**kwargs
)
Expand Down Expand Up @@ -134,11 +129,6 @@ def drake_py_binary(
library code. This prevents submodules from leaking in as top-level
submodules. For more detail, see #8041.
"""

# Work around https://github.com/bazelbuild/bazel/issues/1567.
deps = deps or []
if "//:module_py" not in deps:
deps += ["//:module_py"]
if main == None and len(srcs) == 1:
main = srcs[0]
_py_target_isolated(
Expand Down Expand Up @@ -265,10 +255,7 @@ def drake_py_test(
kwargs = incorporate_num_threads(kwargs, num_threads = num_threads)
kwargs = amend(kwargs, "tags", append = ["py"])

# Work around https://github.com/bazelbuild/bazel/issues/1567.
deps = deps or []
if "//:module_py" not in deps:
deps += ["//:module_py"]
if not allow_import_unittest:
deps = deps + ["//common/test_utilities:disable_python_unittest"]
_py_target_isolated(
Expand Down

0 comments on commit 823f4aa

Please sign in to comment.