[workspace] Update ALIAS_REPOSITORIES to read dicts of aliases - #24916
Conversation
tyler-yankee
left a comment
There was a problem hiding this comment.
holding off on review until the implementation is working...
@tyler-yankee reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on bobpaw).
-- commits line 5 at r1:
nit the phrasing "Now that more" is slightly awkward to me. Consider e.g.:
The assumed `{name: actual}` mapping is inflexible as the number of packages
using this helper grows in the future.
459d13f to
b8dba6c
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 1 file and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on bobpaw).
tools/workspace/default.bzl line 184 at r2 (raw file):
} for name in ALIAS_REPOSITORIES:
nit can we get away with for name, alias in ALIAS_REPOSITORIES.items() in Starlark?
tools/workspace/default.bzl line 185 at r2 (raw file):
for name in ALIAS_REPOSITORIES: alias_root = "@drake//tools/workspace/"
nit I suppose we can define this outside the loop.
tools/workspace/default.bzl line 192 at r2 (raw file):
aliases = {n: alias_root + a for n, a in alias.items()} else: aliases = {name: alias_root + name}
This is never taken, right? Can't weif str ... else assert dict ...?
Code quote:
else:
aliases = {name: alias_root + name}|
Previously, tyler-yankee (Tyler Yankee) wrote…
Will do and I think starlark needs fail not assert |
b8dba6c to
2bb468a
Compare
bobpaw
left a comment
There was a problem hiding this comment.
@bobpaw made 1 comment and resolved 2 discussions.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on tyler-yankee).
tools/workspace/default.bzl line 192 at r2 (raw file):
Previously, bobpaw (Aiden Woodruff) wrote…
Will do and I think starlark needs fail not assert
Done.
The assumed `{name: actual}` mapping is inflexible as the number of
packages using this helper grows in the future. Adjust the logic to
use str for single aliases or dict to map more values.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on bobpaw).
tyler-yankee
left a comment
There was a problem hiding this comment.
+a:@SeanCurtis-TRI for platform review per schedule, please.
@tyler-yankee made 1 comment.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on SeanCurtis-TRI).
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
@SeanCurtis-TRI reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all discussions resolved, LGTM from assignees tyler-yankee,SeanCurtis-TRI(platform) (waiting on bobpaw).
The list of bazel repository aliases used to always be {name: actual}. Now that more will be maps, adjust the logic to support them naturally instead of as edge cases.
re: #24913 (review)
This change is