-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Make File json encodable #26878
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?
Make File json encodable #26878
Conversation
This is useful for printing things as json like cquery output.
Currently this encodes the object as the path directly. I'm not sure if that's accurate enough for all cases? if any of the bools need to be here too we can encode this as a dict instead |
In terms of motivation: this would restore some lost functionality for debugging and comparing CcToolchainConfigInfo's. Prior to moving that provider to Starlark, it used to have a .proto field that encoded the whole thing as a proto. This was valuable for inspecting and comparing. It was the basis of testing when we migrated from Proto to Starlark for crosstool (cc: @hlopko @scentini), and also provided an easy way to consume that data outside of Bazel/Starlark (e.g. in a test). If we were able to json.encode File, then we can effectively json.encode the rest of the CcToolchainConfigInfo (AFAIK that's the only type currently found in common configs that does not presently encode). We should eventually add a test that guarantees that CcToolchainConfigInfos remain encodable going forward. |
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.
In principle, agree.
Currently this encodes the object as the path directly. I'm not sure if that's accurate enough for all cases?
Good question. @comius - should it instead be encoded as {"root": "...", "short_path": "..."}
? Or denormalized but possibly more useful as {"root": "...", "path": "..."}
?
As for implementation - we already have the Json.Encodable
interface, I would suggest using it (and make it take an explicit StarlarkSemantics
) instead of making Json.java
import starlarkbuildapi.
Follow-up: what about label objects?
ah nice thanks I missed this. I switched to that here, although I made it pass through StarlarkSemantics too, since it seems like that's something we need in this case and probably others. Are there internal google implementors of this interface? since there aren't any public ones today. I also noticed that we might want the
probably would be nice to have too. I can implement that if this approach looks good |
This is useful for printing things as json like cquery output.