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] Add boost_internal #21037

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Feb 25, 2024

Also add com_github_nelhage_rules_boost_internal for its BUILD file.

These two repositories are not enabled by default. They are only used when --WITH_USD=ON is enabled.

Towards #20898. Amends #20923.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: medium release notes: fix This pull request contains fixes (no new features) labels Feb 25, 2024
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-release 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.

+@rpoyner-tri for feature review, please.

Reviewed 10 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review February 25, 2024 18:02
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 9 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers


-- commits line 6 at r2:
nit I don't understand this statement. If I pull the branch and run bazel fetch //..., they certainly get fetched. Do you mean they only get built/used when USD is on?

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, needs at least two assigned reviewers


-- commits line 6 at r2:

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit I don't understand this statement. If I pull the branch and run bazel fetch //..., they certainly get fetched. Do you mean they only get built/used when USD is on?

The command fetch is a special, in the sense that it fetches the dependencies of all configurations. The use case when you're about to go offline, e.g., airplane and want to have everything available locally, even as you change configs. It's setting you up to be able to run --WITH_USD={ON,OFF} both ways, while you're on the flight.

For the normal use case, where users and developers only run build //... or test //..., the boost archive will be not downloaded (or unpacked, or ...).

@jwnimmer-tri
Copy link
Collaborator Author

+@sherm1 for platform review per schedule, please.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Platform :lgtm: with a few comments
FYI I didn't see any explicit dependency on WITH_USD here. I presume that's handled automatically by some already-in-place code; just checking.

Reviewed 9 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions


tools/workspace/github.bzl line 403 at r2 (raw file):

        commit = None,
        attachments = None,
        strip_prefix = None,

BTW IIUC this performs double duty: (1) specifies that attachments are archives of some kind that need to have files extracted, and (2) provides a prefix to strip from the resulting file names. Of those I would think function (1) is the most significant but the name hides that. Consider something like extract_and_strip_prefix to make that clearer.


tools/workspace/github.bzl line 422 at r2 (raw file):

        attachments: required dict whose keys are the filenames (attachment
            names) to download and values are the expected SHA-256 checksums.
        strip_prefix: optional dict whose keys are the filenames and values

BTW is the filename key here the same as the filename mentioned two lines above? One is "filenames (attachment names)" and the other just "filenames" which made me think they were different. If they are the same (which I think now) it would be better to describe them identically (or have "strip_prefix" reference "attachment").


tools/workspace/github.bzl line 531 at r2 (raw file):

            repository_ctx.download_and_extract(
                urls,
                stripPrefix = should_strip_prefix,

BTW here "stripPrefix" has only the obvious meaning but the name is nearly identical to "strip_prefix" above which carries the "extract" meaning as well -- I'd prefer more linguistic distance.


tools/workspace/metadata.py line 46 at r2 (raw file):

        # see TODO in tools/workspace/rust_toolchain/repository.bzl.
        repositories.add("bazel_skylib")
        repositories.add("com_github_nelhage_rules_boost_internal")

BTW (just pattern matching here) you added two repositories but only mention one here. I assume that's intentional but thought I should mention it.

Also add com_github_nelhage_rules_boost_internal for its BUILD file.

These two repositories are not enabled by default.
They are only used when --WITH_USD=ON is enabled.
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.

Yup! The spelling on the command line is WITH_USD but in BUILD files we have "//tools:with_usd" as the marker. It's used in tools/workspace/openusd_internal/BUILD.bazel near the end, as part of the sh_test logic.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion


tools/workspace/github.bzl line 403 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW IIUC this performs double duty: (1) specifies that attachments are archives of some kind that need to have files extracted, and (2) provides a prefix to strip from the resulting file names. Of those I would think function (1) is the most significant but the name hides that. Consider something like extract_and_strip_prefix to make that clearer.

For utmost clarity, I've split the two concepts into distinct arguments now.


tools/workspace/github.bzl line 422 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW is the filename key here the same as the filename mentioned two lines above? One is "filenames (attachment names)" and the other just "filenames" which made me think they were different. If they are the same (which I think now) it would be better to describe them identically (or have "strip_prefix" reference "attachment").

A fair point. I sync'd up the text.


tools/workspace/metadata.py line 46 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW (just pattern matching here) you added two repositories but only mention one here. I assume that's intentional but thought I should mention it.

Yup! The only things listed here are things we load() bzl from. Repositories with actual C++ code (like Boost) are located automatically.

Copy link
Member

@sherm1 sherm1 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 r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sherm1(platform)


tools/workspace/github.bzl line 403 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For utmost clarity, I've split the two concepts into distinct arguments now.

Very nice -- thanks!

@jwnimmer-tri jwnimmer-tri merged commit 14f2026 into RobotLocomotion:master Feb 27, 2024
9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the boost branch February 27, 2024 01:22
jwnimmer-tri added a commit to jwnimmer-tri/drake that referenced this pull request Apr 1, 2024
Also add com_github_nelhage_rules_boost_internal for its BUILD file.

These two repositories are not enabled by default.
They are only used when --WITH_USD=ON is enabled.
RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
Also add com_github_nelhage_rules_boost_internal for its BUILD file.

These two repositories are not enabled by default.
They are only used when --WITH_USD=ON is enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants