-
Notifications
You must be signed in to change notification settings - Fork 99
Propagate .jdeps for java_import
#363
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,6 +178,59 @@ def _test_deps_impl(env, targets): | |
| "{package}/depjar.jar", | ||
| ]) | ||
|
|
||
| # Regression test for https://github.com/bazelbuild/rules_java/issues/362: an import with deps | ||
| # records a jdeps proto as its compile_jdeps so that reduced classpaths of downstream compilations | ||
| # are not missing its transitive dependencies. | ||
| def _test_compile_jdeps_propagated_for_deps(name): | ||
| util.helper_target( | ||
| java_import, | ||
| name = name + "/import-jar", | ||
| jars = ["import.jar"], | ||
| deps = [name + "/depjar"], | ||
| ) | ||
| util.helper_target( | ||
| java_import, | ||
| name = name + "/incomplete-jar", | ||
| jars = ["incomplete.jar"], | ||
| deps = [name + "/depjar"], | ||
| tags = ["incomplete-deps"], | ||
| ) | ||
| util.helper_target( | ||
| java_import, | ||
| name = name + "/depjar", | ||
| jars = ["depjar.jar"], | ||
| ) | ||
|
|
||
| analysis_test( | ||
| name = name, | ||
| impl = _test_compile_jdeps_propagated_for_deps_impl, | ||
| targets = { | ||
| "with_deps": name + "/import-jar", | ||
| "without_deps": name + "/depjar", | ||
| }, | ||
| # Requires the rules_java Starlark implementation to be used. | ||
| attr_values = {"tags": ["min_bazel_9"]}, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. starlark impl is used from Bazel 8 |
||
| ) | ||
|
|
||
| def _test_compile_jdeps_propagated_for_deps_impl(env, targets): | ||
| with_deps = [ | ||
| f.basename | ||
| for f in targets.with_deps[JavaInfo]._compile_time_java_dependencies.to_list() | ||
| ] | ||
| env.expect.that_collection(with_deps).contains_exactly(["jdeps.proto"]) | ||
|
Comment on lines
+216
to
+220
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. written this way, the test will have terrible failure messages if/when it fails. use (same below) |
||
|
|
||
| incomplete_deps = [ | ||
| f.basename | ||
| for f in targets.without_deps[JavaInfo]._compile_time_java_dependencies.to_list() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not the |
||
| ] | ||
| env.expect.that_collection(incomplete_deps).contains_exactly(["jdeps.proto"]) | ||
|
|
||
| without_deps = [ | ||
| f.basename | ||
| for f in targets.without_deps[JavaInfo]._compile_time_java_dependencies.to_list() | ||
| ] | ||
| env.expect.that_collection(without_deps).contains_exactly(["jdeps.proto"]) | ||
|
|
||
| # Regression test for b/262751943. | ||
| def _test_commandline_contains_target_label(name): | ||
| util.helper_target( | ||
|
|
@@ -959,6 +1012,7 @@ def java_import_tests(name): | |
| _test_simple, | ||
| _test_with_java_library, | ||
| _test_deps, | ||
| _test_compile_jdeps_propagated_for_deps, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think tests for the |
||
| _test_commandline_contains_target_label, | ||
| _test_java_library_allows_import_in_deps, | ||
| _test_module_flags, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,7 @@ _DEFAULT_JAVA_LANGUAGE_VERSION = "11" | |
|
|
||
| # Default java_toolchain parameters | ||
| _BASE_TOOLCHAIN_CONFIGURATION = dict( | ||
| deps_checker = Label("@remote_java_tools//:ImportDepsChecker"), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change for Bazel as the hardcoded mode is Are you precompiling the tool to a native image at Google?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's a prebuilt jar that runs on the JVM, but it's also a validation action that is not on the critical path (#362 (comment)). Using a native image seems like a good idea if it's going to run as a non-validation action so the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Bazel supports cc @hvadehra
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My main concern is that external repos using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should only be a breaking change for Bazel 7, which is okay I guess. I also don't see why we need a separate switch - one can just unset |
||
| forcibly_disable_header_compilation = False, | ||
| genclass = Label("@remote_java_tools//:GenClass"), | ||
| header_compiler = Label("@remote_java_tools//:TurbineDirect"), | ||
|
|
||
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.
unused?