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

Tidy up computation of import paths #182

Merged

Conversation

EdSchouten
Copy link
Collaborator

Right now you see that jsonnet_to_json() and jsonnet_to_json_test() implicitly call jsonnet with "-J .". This means that if these rules are used in combination with libraries that are declared in the root module, everything works as expected. But as soon as libraries are used from other modules, importing them becomes more tedious.

The goal of this change is to ensure that if a Jsonnet project can be built within the root module, that module can also safely be used as a child module. We solve this by automatically adding the workspace root to the set of import paths for which one or more source files exist. Furthermore, by considering the root of every source file, we no longer need to use bin_dir and genfiles_dir.

Fixes: #44
Fixes: #86
Fixed: #139
Fixes: #154
Fixes: #178

Right now you see that jsonnet_to_json() and jsonnet_to_json_test()
implicitly call jsonnet with "-J .". This means that if these rules are
used in combination with libraries that are declared in the root module,
everything works as expected. But as soon as libraries are used from
other modules, importing them becomes more tedious.

The goal of this change is to ensure that if a Jsonnet project can be
built within the root module, that module can also safely be used as a
child module. We solve this by automatically adding the workspace root
to the set of import paths for which one or more source files exist.
Furthermore, by considering the root of every source file, we no longer
need to use bin_dir and genfiles_dir.

Fixes: bazel-contrib#44
Fixes: bazel-contrib#86
Fixed: bazel-contrib#139
Fixes: bazel-contrib#154
Fixes: bazel-contrib#178
@EdSchouten EdSchouten force-pushed the eschouten/20240404-externals branch from 91d5be0 to 2fbc5a6 Compare April 4, 2024 09:59
@EdSchouten EdSchouten merged commit 0a60c2f into bazel-contrib:master Apr 4, 2024
2 checks passed
@EdSchouten EdSchouten deleted the eschouten/20240404-externals branch April 4, 2024 10:00
EdSchouten added a commit that referenced this pull request Apr 4, 2024
It looks like #182 introduced a small regression, where '%' was used
instead of '+' to perform string concatenation. This went by unnoticed,
because we seemingly don't have any testing coverage for plain
jsonnet_to_json().

Solve this by duplicating one of the import tests that was affected by
this to invoke plain jsonnet_to_json().
EdSchouten added a commit that referenced this pull request Apr 4, 2024
It looks like #182 introduced a small regression, where '%' was used
instead of '+' to perform string concatenation. This went by unnoticed,
because we seemingly don't have any testing coverage for plain
jsonnet_to_json().

Solve this by duplicating one of the import tests that was affected by
this to invoke plain jsonnet_to_json().
["-J %s" % im for im in depinfo.imports.to_list()] + [
"-J .",
"-J %s" % ctx.genfiles_dir.path,
"-J %s" % ctx.bin_dir.path,
Copy link

Choose a reason for hiding this comment

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

@EdSchouten - the removal of the bin_dir is a divergence in behavior from the past. Do you feel that this is more correct?

(I'm encountering an issue related to this in a large repo - but wanting to understand if the change is intentional)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bindir should still be added for generated source files, through file.root.path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you make a small reproduction case?

@frimik
Copy link

frimik commented Sep 30, 2024

Something here seems to have broken imports for all our rules relying on imports being populated from a couple of core deps. Now despite "top-level" deps having for example; imports = ["lib", "vendor"] the only thing showing up in the jsonnet command are the two duplicated . arguments, like: -J . -J ..

lib/kube-libsonnet/BUILD:

jsonnet_library(
    name = "kube",
    srcs = ["kube.libsonnet"],
    visibility = ["//visibility:public"],
    deps = ["//:imports"], # this doesn't work anymore ...
    imports = ["../"] # this had to be added now
)

With the explicit `imports = ["../"] in this library, it works.

The trickery I did in previous versions was a top-level //:imports rule which was a dep of the other libs, see example:

jsonnet_library(
    name = "imports",
    imports = [
        "./lib",
        "./vendor",
    ],
)

jsonnet_library(
    name = "k",
    srcs = glob([
        "vendor/github.com/jsonnet-libs/k8s-libsonnet/1.25/**/*.libsonnet",
        "lib/k/**/*.libsonnet",
    ]) + ["lib/k.libsonnet"],
    visibility = ["//visibility:public"],
    deps = [
        ":imports",
        ":ksonnet-util",
    ],
)

Why? This was to make other jsonnet tools and bazel able to co-exist. Many of those tools, and editor configurations expect the import paths to be -J vendor -J lib to have good mechanisms for both imports and overrides available.

@EdSchouten
Copy link
Collaborator Author

EdSchouten commented Sep 30, 2024

The reason this construct no longer works is becasue jsonnet_library(name = "imports") does not have any sources. Without sources, the rule isn't able to determine whether it should add include paths for the source directory or the output directory.

Instead of using that dummy jsonnet_library(name = "imports"), why can't you add imports = [] to jsonnet_library(name = "k") directly? If you want to reuse these import paths between various targets in that same package, keep in mind that you can always put those in a constant MY_IMPORTS = ["lib", "vendor"] and use that constant within your BUILD file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants