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

Add annotation support for transient crates. #2392

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

matts1
Copy link
Contributor

@matts1 matts1 commented Jan 4, 2024

No description provided.

@matts1 matts1 force-pushed the annotations branch 2 times, most recently from 991b359 to a21c92c Compare January 4, 2024 02:36
@matts1 matts1 marked this pull request as ready for review January 4, 2024 02:37
@matts1
Copy link
Contributor Author

matts1 commented Jan 4, 2024

@illicitonion I've done a complete rewrite of annotations according to your suggestions of inlining them into MODULE.bazel.

@gferon
Copy link
Contributor

gferon commented Jan 4, 2024

I tested this PR on our current setup (still trying to move to bzlmod) and it fails on json.encode when supplying labels like build_script_tools = ["@com_google_protobuf//:protoc"] with the following error:

(11:07:32) ERROR: Traceback (most recent call last):
        File "/Users/gferon/.cache/bazel/8d38801da57cbc2fb5fa58e853c562bd/external/rules_rust~override/crate_universe/extension.bzl", line 182, column 58, in _crate_impl
                annotation = _crate_universe_crate.annotation(**{
        File "/Users/gferon/.cache/bazel/8d38801da57cbc2fb5fa58e853c562bd/external/rules_rust~override/crate_universe/private/crate.bzl", line 179, column 23, in _annotation
                return json.encode((
Error in encode: at tuple index 1: in struct field .build_script_tools: at list index 0: cannot encode Label as JSON

I'm not sure yet what the right fix would be though.

EDIT: I tried to "stringify" those labels, but the produced JSON can't be parsed anymore by cargo-bazel, so I'm definitely doing something wrong here.

crate_universe/extension.bzl Outdated Show resolved Hide resolved
illicitonion added a commit to illicitonion/rules_rust that referenced this pull request Jan 4, 2024
This is needed for bazelbuild#2392 as
starlark doesn't JSON-serialize labels.
@illicitonion
Copy link
Collaborator

I tested this PR on our current setup (still trying to move to bzlmod) and it fails on json.encode when supplying labels like build_script_tools = ["@com_google_protobuf//:protoc"] with the following error:

(11:07:32) ERROR: Traceback (most recent call last):
        File "/Users/gferon/.cache/bazel/8d38801da57cbc2fb5fa58e853c562bd/external/rules_rust~override/crate_universe/extension.bzl", line 182, column 58, in _crate_impl
                annotation = _crate_universe_crate.annotation(**{
        File "/Users/gferon/.cache/bazel/8d38801da57cbc2fb5fa58e853c562bd/external/rules_rust~override/crate_universe/private/crate.bzl", line 179, column 23, in _annotation
                return json.encode((
Error in encode: at tuple index 1: in struct field .build_script_tools: at list index 0: cannot encode Label as JSON

I'm not sure yet what the right fix would be though.

EDIT: I tried to "stringify" those labels, but the produced JSON can't be parsed anymore by cargo-bazel, so I'm definitely doing something wrong here.

#2394 should fix this

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks! Just one comment around booleans, then I think we're good to go.

crate_universe/extension.bzl Show resolved Hide resolved
illicitonion added a commit to illicitonion/rules_rust that referenced this pull request Jan 4, 2024
This is needed for bazelbuild#2392 as
starlark doesn't JSON-serialize labels.
illicitonion added a commit to illicitonion/rules_rust that referenced this pull request Jan 5, 2024
This is needed for bazelbuild#2392 as
starlark doesn't JSON-serialize labels.
illicitonion added a commit that referenced this pull request Jan 5, 2024
This is needed for #2392 as
starlark doesn't JSON-serialize labels.
@matts1
Copy link
Contributor Author

matts1 commented Jan 8, 2024

I've just rebased. There was also apparently some problems with using relative labels, as they were being evaluated relative to the wrong package. I've fixed it up now by turning them into strings.

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Thanks so much!

@illicitonion illicitonion enabled auto-merge (squash) January 8, 2024 11:24
@illicitonion illicitonion merged commit db03e3e into bazelbuild:main Jan 8, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants