Add support for extvars in jsonnet_library

[dbanetto]

Intent

Implement the suggestion from #89 to resolve a problem of needing to reference generated files from deep in a jsonnet_library dependency graph.

A concrete use case that this could help is having a jsonnet_library that loads an external file, such as https://github.com/jsonnet-libs/k8s-libsonnet . To use this library internally you need to have the jsonnet code similar to:

jsonnet_library(
  name = "k8s-libsonnet",
  srcs = ["main.libsonnet"],
  deps = [
    "@com_github_jsonnet_libs_k8s_libjsonnet//:files",
  ],
)

// main.libsonnet
import 'external/com_github_jsonnet_libs_k8s_libjsonnet/main.jsonnet'

The above being for a WORKSPACE file, the will change depending how it is setup as a Bazel Mod and the Bazel version used.

With this change, this could be changed to:

jsonnet_library(
  name = "k8s-libsonnet",
  srcs = ["main.libsonnet"],
  ext_code_files = {
    "@com_github_jsonnet_libs_k8s_libjsonnet//:files": "k8s_libsonnet_main"
  },
)

// main.libsonnet
std.extVar('k8s_libsonnet_main')

Another use case, as the original author of #89 described, use of this is including generated files with std.extVar instead of using importstr.

Design

The design of this implementation revolves around a extvar_dict which gathers the required information about extvar's from the deps tree of jsonnet_library and make them compatible with jsonnet_to_json{,_test}.

Implementation Notes

To do this, the implementation adds a new field to JsonnetLibraryInfo, transistive_extvars, which is a dictionary that is keyed on extvar name and contains a dict that contains:

{
  "type": str, // code, string, code_file, string_file, code_env, string_env, code_library
  "value": File, // File to use
  "sources": List[str] // stringified labels for user friendly error reports
}

It also added three new functions to help handle extvars from across jsonnet_library and how they propagate through deps to the final jsonnet_to_json{,_test} targets:

1. _make_extvar_dict whose goal is to convert the ctx.attr's into a uniform dict
2. _merge_extvars whose goal is to combine multiple extvar_dict and ensure that duplicates are handled correctly
3. _extvar_to_arguments whose goal is to convert back from the dict to string arguments

This has the behaviour of ensuring that the same extvar cannot be defined multiple times with different values. It does allow the same extvars with the same value to be defined in multiple places to ensure having a common dependency across a dependency tree works. However, it does allow two different targets to define the same extvar in two places to keep in sync.

[gemini-code-assist]

Summary of Changes

Hello @dbanetto, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the jsonnet_library Bazel rule by introducing robust support for Jsonnet external variables (extvars). It addresses the challenge of referencing generated files or external libraries deep within a jsonnet_library dependency graph by allowing extvars to be defined and propagated transitively. This change provides a more streamlined and powerful mechanism for integrating external data and code into Jsonnet builds, improving flexibility and reducing boilerplate.

Highlights

• Enhanced jsonnet_library Rule: The jsonnet_library rule now supports a comprehensive set of extvar attributes, including ext_code, ext_code_envs, ext_code_files, ext_code_libraries, ext_str_envs, ext_str_files, and ext_strs, allowing for flexible external variable definition.
• Transitive External Variable Propagation: A new transitive_extvars field has been added to JsonnetLibraryInfo to enable the propagation and merging of external variables across the dependency graph, ensuring consistent extvar definitions throughout a Jsonnet project.
• Robust extvar Handling Logic: Three new functions (_make_extvar_dict, _merge_extvars, _extvar_to_arguments) were introduced to manage the creation, merging (with conflict detection for duplicates), and conversion of extvars into command-line arguments for the Jsonnet compiler.
• Updated jsonnet_to_json and jsonnet_to_json_test: The jsonnet_to_json and jsonnet_to_json_test rules have been updated to leverage the new transitive extvar information, simplifying the process of passing external variables to Jsonnet evaluation.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces support for transitive extvars in jsonnet_library, which is a great feature for managing dependencies. The implementation is well-structured with helper functions for creating, merging, and converting extvars.

I've found a critical issue with the propagation of transitive extvars in jsonnet_library, a high-severity issue with how file-based extvars with multiple files are handled, and a suggestion to improve maintainability by reducing code duplication. Please see my detailed comments.

Once these issues are addressed, this will be a solid contribution.

[gemini-code-assist]

The logic for handling file-based extvars (ext_code_files, ext_code_libraries, ext_str_files) has a flaw. When the backing target (e.g., a filegroup or jsonnet_library) contains multiple files, your code stores a list of files, but _extvar_to_arguments later silently uses only the first file. This can cause subtle bugs.

Since the jsonnet compiler only supports a single file per --ext-*-file flag for a given variable, you should enforce that the backing targets for these attributes provide exactly one file and fail the build otherwise.

You should also store the single File object directly in the value field instead of a list containing one file. This will require a small change in _extvar_to_arguments to access val["value"] instead of val["value"][0].

Here's how you could adjust the logic for ext_code_files:

    for file, key in ext_code_files.items():
        if key in extvars:
            fail("duplicate extVar '{}': {}".format(key, extvars.keys()))

        files = [file]
        if type(file) != "File":
            files = file[DefaultInfo].files.to_list()

        if len(files) != 1:
            fail("For ext_code_files, the target for extvar '{}' in '{}' must contain exactly one file, but it contained {}.".format(key, label, len(files)))

        extvars[key] = {
            "value": files[0],
            "type": "code_file",
            "sources": [label],
        }

Similar changes should be applied to ext_code_libraries and ext_str_files.

[dbanetto]

Since the jsonnet compiler only supports a single file per --ext-*-file flag for a given variable, you should enforce that the backing targets for these attributes provide exactly one file and fail the build otherwise.

It has hit the nail on the head here, though as the current rules_jsonnet already has this behaviour in:

rules_jsonnet/jsonnet/jsonnet.bzl

Lines 472 to 475 in d303289

	["--tla-str-file %s=%s" %
	(var, jfile.files.to_list()[0].short_path) for jfile, var in jsonnet_tla_str_files.items()] +
	["--tla-code-file %s=%s" %
	(var, jfile.files.to_list()[0].short_path) for jfile, var in jsonnet_tla_code_files.items()] +

I wouldn't want to add this has a new failure, this will cause existing code to break.

[EdSchouten]

This all looks very convoluted. All of the type strings, checking against the output of type(x), etc.

What's the reason we can't just extend JsonnetLibraryInfo to have individual fields for each of these options?

[dbanetto]

My rationale for going down this path is to make verifying that the extvar variable has not been defined & given different values as simple as possible.

The implementation is trading complexity in _make_extvar_dict & _extvar_to_arguments to handle the format that is optimised for _merge_extvars to reduce the likelihood of an incorrect transitive_extvars is made.

I think this would also come with some good DX as error messages can better identify & explain sources of duplication of extvar variables.

What's the reason we can't just extend JsonnetLibraryInfo to have individual fields for each of these options?

After playing with the idea for a bit, it puts more complexity into _merge_extvars to handle each of the new fields & feels more error-prone. With _make_extvar_dict & _extvar_to_arguments not changing too much as they plumb through each field.

---

With that experiment, I was able to find a refactor (87392a7) to remove the need for of the {string,code}_* variants and reduce type down to being string or code which also lead to some (probably overzealous) use of zip() for the quoted section.