Use consistent include paths when buiding in an external repository #178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agotsis
commented
Apr 1, 2024
| doc = "List of import `-J` flags to be passed to the `jsonnet` compiler.", | ||
| doc = """List of import `-J` flags to be passed to the `jsonnet` compiler. | ||
|
|
||
| The workspace_root, the genfiles_dir, and bin_dir are always included.""", |
There was a problem hiding this comment.
This was already true. The generated documentation is not updated due to #177.
agotsis
force-pushed
the
external-repo-compatibility
branch
from
0af6a6c to
3697af1
Compare
EdSchouten
added a commit
to EdSchouten/rules_jsonnet
that referenced
this pull request
Apr 4, 2024
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
added a commit
to EdSchouten/rules_jsonnet
that referenced
this pull request
Apr 4, 2024
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
|
Thanks for the PR, @agotsis! I just merged a somewhat different change in #182 that should also address this issue. The reason I deviated from your change is because I think that we should simply make sure that the |
|
Thank you very much @EdSchouten - I have tested your changes in a large repo with complex dependencies on jsonnet and found that the issue that motivated me to open the PR has been fixed <3 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the include paths passed to the jsonnet compiler are not all adjusted for when used in an external repository - only explicitly included imports.
This results in inconsistent include behavior when rules from rules_jsonnet are depended upon in an external repository.
Adding the workspace_root as appropriate makes this behavior consistent between builds in a local workspace and in an external one.
All existing tests pass
bazel build @examples//...with bazel 6.4.0.