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

[build] Use MODULE.bazel for all dependencies #22338

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Dec 20, 2024

Towards #20731.

The WORKSPACE.bzlmod is no longer load-bearing, so as of this commit it should be possible to consume Drake as module external of other downstream projects (instead of as a repository external).

See RobotLocomotion/drake-external-examples#348 for how to use this downstream.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added the release notes: feature This pull request contains a new feature label Dec 20, 2024
jwnimmer-tri

This comment was marked as resolved.

jwnimmer-tri

This comment was marked as resolved.

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review December 21, 2024 21:10
@jwnimmer-tri jwnimmer-tri force-pushed the bzlmod-extension branch 2 times, most recently from 934fb04 to 4cefac6 Compare January 3, 2025 15:42
@jwnimmer-tri jwnimmer-tri force-pushed the bzlmod-extension branch 2 times, most recently from 05fd5e4 to a664d58 Compare January 7, 2025 21:12
@xuchenhan-tri xuchenhan-tri self-assigned this Jan 7, 2025
@xuchenhan-tri
Copy link
Contributor

+@xuchenhan-tri for feature review.

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.

Reviewed 2 of 2 files at r5, 3 of 18 files at r7, 18 of 18 files at r8, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers


tools/install/install.bzl line 534 at r8 (raw file):

        content = "\n".join([
            "#!/bin/bash",
            "{} --actions '{}' \"$@\"".format(

FYI This is required because of how our CMake install is coming from a different MODULE now. We can't hard-code the path anymore.

jwnimmer-tri

This comment was marked as resolved.

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.

Timed out for the day. I'm slowly working through this. It is taking longer than I expected, and I'll have to come back to this tomorrow.

Reviewed 2 of 10 files at r2, 1 of 7 files at r3, 1 of 2 files at r4, 2 of 2 files at r5, 1 of 18 files at r7, 12 of 18 files at r8, 2 of 4 files at r9, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers


-- commits line 6 at r8:
BTW, I assume RobotLocomotion/drake-external-examples#348 provides test coverage for this?

Code quote:

  The WORKSPACE.bzlmod is no longer load-bearing, so as of this commit
  it should be possible to consume Drake as a module external of other
  downstream projects (instead of as a repository external).

WORKSPACE.bzlmod line 5 at r8 (raw file):

# This file lists Drake's workspace-style external dependencies that are only
# needed by Drake Developers instead of downstream projects. Most dependencies
# are listed in MODULE.bazel, instead.

BTW, IIUC, the intent is that this file should only shrink, but never grow, in size. Consider making that point clearer.

Suggestion:

# needed by Drake Developers instead of downstream projects. All other dependencies
# are listed in MODULE.bazel, instead.

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 xuchenhan-tri(platform), needs at least two assigned reviewers


-- commits line 6 at r8:

Previously, xuchenhan-tri wrote…

BTW, I assume RobotLocomotion/drake-external-examples#348 provides test coverage for this?

Yes, exactly right. That's the most relevant / important test of this feature.

To a certain extent, our pre-merge CMake builds also provide coverage because they create a new dummy module ("drake_cmake") which consumes the "drake" module as a bzlmod external.


WORKSPACE.bzlmod line 5 at r8 (raw file):

Previously, xuchenhan-tri wrote…

BTW, IIUC, the intent is that this file should only shrink, but never grow, in size. Consider making that point clearer.

Working

In fact, I should probably take it a step further and make this file only for CLion Drake Developers and disable off the workspace entirely in our default rcfile.

I will push on that tomorrow.

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.

Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers


WORKSPACE.bzlmod line 5 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

In fact, I should probably take it a step further and make this file only for CLion Drake Developers and disable off the workspace entirely in our default rcfile.

I will push on that tomorrow.

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:

Reviewed 3 of 17 files at r1, 1 of 10 files at r2, 1 of 7 files at r3, 2 of 18 files at r7, 6 of 18 files at r8, 2 of 4 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status: 6 unresolved discussions, needs at least two assigned reviewers


MODULE.bazel line 32 at r10 (raw file):

    "cc_configure_extension",
)
use_repo(cc_configure, "local_config_cc")

BTW, where and how is the local_config_cc repo used now that the cc external is deprecated?


MODULE.bazel line 88 at r10 (raw file):

    internal_repositories,
    "abseil_cpp_internal",
    "bazelisk",

BTW, I realize that some of these repos don't have the suffix _internal. Do we have plans to make the names consistent?


tools/install/libdrake/header_lint.bzl line 15 at r10 (raw file):

    "+drake_dep_repositories+eigen",
    "+drake_dep_repositories+fmt",
    "+drake_dep_repositories+lcm",

BTW, I don't see lcm in the header_dependency_test. Should it be added?

Also, whoever came up with this spelling with +'s definitely managed to trip me over it everytime I look at it.


tools/workspace/default.bzl line 121 at r10 (raw file):

    """Declares workspace repositories for all externals needed by drake (other
    than those built into Bazel, of course).  This is intended to be loaded and
    called from a WORKSPACE file.

BTW, this is now also called in the implementation of the module extension.

Code quote:

    than those built into Bazel, of course).  This is intended to be loaded and
    called from a WORKSPACE file.

tools/workspace/workspace_bzlmod_sync_test.py line 144 at r10 (raw file):

    def test_default_exported_sync(self):
        """Our default.bzl has a list of REPOS_EXPORTED that must match the

BTW, what about internal repos? Do we want to check that the repos used through MODULE and WORKSPACE match? Or is the fact that things compile is good enough?


tools/workspace/pybind11/BUILD.bazel line 20 at r10 (raw file):

# This alias provides a single point of control for defining which pybind11
# library our tools/skylark/pybind.bzl macro should use.
alias(

BTW, how do we be sure that this is the only symbol that causes issue when switching to bzlmod? Through tests in drake_external_examples?

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.

Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers


MODULE.bazel line 32 at r10 (raw file):

Previously, xuchenhan-tri wrote…

BTW, where and how is the local_config_cc repo used now that the cc external is deprecated?

The effect of this call is to register a C++ toolchain with Bazel, which ends up being the highest priority toolchain and thus implicitly used by rules like cc_library.


MODULE.bazel line 88 at r10 (raw file):

Previously, xuchenhan-tri wrote…

BTW, I realize that some of these repos don't have the suffix _internal. Do we have plans to make the names consistent?

Added a TODO. My thinking it to not really worry about it until after we've deprecate WORKSPACE mode entirely.


tools/install/libdrake/header_lint.bzl line 15 at r10 (raw file):

Previously, xuchenhan-tri wrote…

BTW, I don't see lcm in the header_dependency_test. Should it be added?

Also, whoever came up with this spelling with +'s definitely managed to trip me over it everytime I look at it.

Explained with new comment.


tools/workspace/workspace_bzlmod_sync_test.py line 144 at r10 (raw file):

Previously, xuchenhan-tri wrote…

BTW, what about internal repos? Do we want to check that the repos used through MODULE and WORKSPACE match? Or is the fact that things compile is good enough?

My belief is that for internal repos, any kind of discrepancy will immediately result in a build error, so we don't need a linter.


tools/workspace/pybind11/BUILD.bazel line 20 at r10 (raw file):

Previously, xuchenhan-tri wrote…

BTW, how do we be sure that this is the only symbol that causes issue when switching to bzlmod? Through tests in drake_external_examples?

Yes. Testing with DEE and then also with Anzu.

Our https://drake.mit.edu/stable.html API forbids people from using nearly all of our skylark macros in the first place (other than repository rules), so really nobody was supposed to be subject to being broken here anyway. However we had a few place in DEE and Anzu that were using pybind.bzl outside of the Stable API, so it seemed easy enough to keep that working.

Sometime later this year I hope to pull pybind11 from BCR anyway (if we're still using it) and there it comes along with a ruleset already, so hopefully this all becomes moot.

@rpoyner-tri rpoyner-tri self-assigned this Jan 8, 2025
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.

+@rpoyner-tri for platform review.

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

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 3 of 3 files at r11, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)

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.

checkpoint -- more later.

Reviewed 2 of 2 files at r5, 1 of 18 files at r7, 7 of 18 files at r8, 1 of 4 files at r9, 1 of 3 files at r11, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)


MODULE.bazel line 217 at r11 (raw file):

# TODO(#20731) More improvements are still needed to our MODULE organization:
# - Switch public API dependencies (e.g., eigen) to use modules.
# - Provide better configuation options for choosing dependencies.

typo

Suggestion:

configuration

tools/workspace/default.bzl line 115 at r11 (raw file):

# =============================================================================
# For Bazel projects using Drake as a depedency via the WORKSPACE mechanism.

typo

Suggestion:

dependency

tools/workspace/default.bzl line 397 at r11 (raw file):

# =============================================================================
# For Bazel projects using Drake as a depedency via the MODULE mechanism.

typo

Suggestion:

dependency

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.

Reviewed 1 of 18 files at r8, 1 of 4 files at r10.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)

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 2 of 17 files at r1, 3 of 10 files at r2, 1 of 7 files at r3, 2 of 18 files at r7, 5 of 18 files at r8, 2 of 4 files at r9, 3 of 4 files at r10, 2 of 3 files at r11.
Reviewable status: 3 unresolved discussions

The WORKSPACE.bzlmod is no longer load-bearing, so as of this commit
it should be possible to consume Drake as a module external of other
downstream projects (instead of as a repository external).

Notable changes:

Document our stability promises for our module extension.

Adjust CMake logic to use its own MODULE.bazel.in that consumes Drake
as a module external, and to override choices for other dependencies
using conventional Bazel command-line flags instead of editing the
WORKSPACE file.

Adjust the pkg_config repository rule to handle the new distinction
between canonical vs apparent repository names. Ditto for the java
maven repositories (only relevant on macOS).

Adjust our lcm native code loader to accommodate the new runfiles
layout. Add test coverage for EventLog (which uses a distinctive
spelling of its import paths in upstream code).

Adjust labels used by our (non-symbolic) pybind11 macro to only ever
refer to the drake label, not the external label. Textual macros
resolve labels in the workspace context of the code calling them, not
Drake. Therefore they must only ever refer to Drake, since Drake's
externals are now invisible (by default) with bzlmod. We introduce
Drake aliases for the externals so that we can use a safe labels in
our macros. (This fix is only necessary for macros which we expect
downstream code to call, i.e., macros without a "drake_..." prefix in
their name. We still have plenty of other drake-specific macros that
refer to non-drake labels, but that's not a problem.) The longer term
fix for this will probably be switching from textual macros to
symbolic macros, but we don't attempt that here.

Adjust some of our hard-coded runfiles paths (header_lint test, wheel
build snopt, drake_models parse_test) to align with the new canonical
repository names.
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.

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),xuchenhan-tri(platform)

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.

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),xuchenhan-tri(platform)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants