-
Notifications
You must be signed in to change notification settings - Fork 945
[bazel] Normalize local labels to //
#29192
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
base: master
Are you sure you want to change the base?
Conversation
These refer to the root module which may not be OpenTitan when used as a dependency. All of these labels should refer to the *current* module so I have updated them to `//`. Signed-off-by: James Wainwright <james.wainwright@lowrisc.org>
Naming the current module is not necessary, `//` will select the current module. Signed-off-by: James Wainwright <james.wainwright@lowrisc.org>
|
|
||
| return select({ | ||
| "@lowrisc_opentitan//hw/top:is_{}".format(top): obj | ||
| "//hw/top:is_{}".format(top): obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This @lowrisc_opentitan here is useful because this file may end up getting imported in other repositories (if we ever managed to make the repo fully use-able as a dependency). In a .bzl file, the local labels refer to the repository of the BUILD file importing it, not the repository to which the .bzl file belongs.
| tops = [tops] | ||
| for top in tops: | ||
| branches["@lowrisc_opentitan//hw/top:is_{}".format(top)] = value | ||
| branches["//hw/top:is_{}".format(top)] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, applies to the whole file.
| @@ -9,17 +9,17 @@ DARJEELING_OTP_SIGVERIFY_FAKE_KEYS = [ | |||
| # Additional overlays can be applied on top to further customize the OTP. | |||
| # This set of overlays does not include any of the SECRET[0-2] partitions. | |||
| DARJEELING_STD_OTP_OVERLAYS_WITHOUT_SECRET_PARTITIONS = DARJEELING_OTP_SIGVERIFY_FAKE_KEYS + [ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure but I think due to the way those defs.bzl files end-up being imported by the multi-top build system, switching that to @lowrisc_opentitan would be more correct. This applies to any construct which ends up in the opentitan_top declaration.
| runtime_deps = [ | ||
| "@//sw/device/silicon_creator/lib/cert:asn1", | ||
| "@//sw/device/silicon_creator/lib/cert:template", | ||
| "//sw/device/silicon_creator/lib/cert:asn1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is used out of tree (which would make sense), this needs to be @lowrisc_opentitan. Same for the rest of the file.
|
I have left some comments but they are probably incomplete. Essentially any .bzl file which may potentially be imported by another repository (if we support fully the repo being a dependency) should probably use |
|
Oh, I see, when |
Replaces uses of
@//and@lowrisc_opentitan//with//. The first form references the root module when OpenTitan is used as a dependency, which is not what we want. The second form is fine but longer than necessary.