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

final phase is inlined #947

Merged
merged 1 commit into from
Jan 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/customizable_phase.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,14 @@ Currently phase architecture is used by 7 rules:
- scala_junit_test
- scala_repl

In each of the rule implementation, it calls `run_phases` and returns the information from `phase_final`, which groups the final returns of the rule. To prevent consumers from accidently removing `phase_final` from the list, we make it a non-customizable phase.
If you need to expose providers to downstream targets you need to return an array of providers from your phase under the `external_providers` attribute.

In each of the rule implementations, it calls `run_phases` and returns an accumulated `external_providers` array declared by the phases.

To make a new phase, you have to define a new `phase_<PHASE_NAME>.bzl` in `scala/private/phases/`. Function definition should have 2 arguments, `ctx` and `p`. You may expose the information for later phases by returning a `struct`. In some phases, there are multiple phase functions since different rules may take slightly different input arguemnts. You may want to re-expose the phase definition in `scala/private/phases/phases.bzl`, so it's more convenient to access in rule files.

In the rule implementations, put your new phase in `builtin_customizable_phases` list. The phases are executed sequentially, the order matters if the new phase depends on previous phases.

If you are making new return fields of the rule, remember to modify `phase_final`.

### Phase naming convention
Files in `scala/private/phases/`
- `phase_<PHASE_NAME>.bzl`: phase definition file
Expand Down
6 changes: 3 additions & 3 deletions scala/private/phases/api.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def _adjust_phases(phases, adjustments):
return phases

# Execute phases
def run_phases(ctx, builtin_customizable_phases, fixed_phase):
def run_phases(ctx, builtin_customizable_phases):
# Loading custom phases
# Phases must be passed in by provider
phase_providers = [
Expand All @@ -64,7 +64,7 @@ def run_phases(ctx, builtin_customizable_phases, fixed_phase):
global_provider = {}
current_provider = struct(**global_provider)
acculmulated_external_providers = []
for (name, function) in adjusted_phases + [fixed_phase]:
for (name, function) in adjusted_phases:
# Run a phase
new_provider = function(ctx, current_provider)

Expand All @@ -77,7 +77,7 @@ def run_phases(ctx, builtin_customizable_phases, fixed_phase):
current_provider = struct(**global_provider)

# The final return of rules implementation
return acculmulated_external_providers + current_provider.final
return acculmulated_external_providers

# A method to pass in phase provider
def extras_phases(extras):
Expand Down
1 change: 1 addition & 0 deletions scala/private/phases/phase_collect_jars.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ def _phase_collect_jars(
transitive_compile_jars = transitive_compile_jars,
transitive_runtime_jars = transitive_rjars,
deps_providers = deps_providers,
external_providers = [jars2labels],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, phase_compile.bzl is using jars2labels, but we also put jars2labels in external_providers. Can we simplify it?

)

def _collect_runtime_jars(dep_targets):
Expand Down
1 change: 1 addition & 0 deletions scala/private/phases/phase_compile.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ def _phase_compile(
java_jar = out.java_jar,
source_jars = _pack_source_jars(ctx) + out.source_jars,
merged_provider = out.merged_provider,
external_providers = [out.merged_provider] + out.coverage.providers,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a duplicate from merged_provider = out.merged_provider, but it turns out phase_jvm_flags.bzl is using p.compile.merged_provider. Is it possible to simplify it?

)

def _compile_or_empty(
Expand Down
36 changes: 36 additions & 0 deletions scala/private/phases/phase_default_info.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#
# PHASE: default_info
#
# DOCUMENT THIS
#
def phase_binary_default_info(ctx, p):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as in #948:

Could we name these phase_default_info_binary, phase_default_info_library, and phase_default_info_scalatest instead? I think it really improves code navigability when the file name is the prefix for all exported functions.

return struct(
external_providers = [
DefaultInfo(
executable = p.declare_executable,
files = depset([p.declare_executable] + p.compile.full_jars),
runfiles = p.runfiles.runfiles,
),
],
)

def phase_library_default_info(ctx, p):
return struct(
external_providers = [
DefaultInfo(
files = depset(p.compile.full_jars),
runfiles = p.runfiles.runfiles,
),
],
)

def phase_scalatest_default_info(ctx, p):
return struct(
external_providers = [
DefaultInfo(
executable = p.declare_executable,
files = depset([p.declare_executable] + p.compile.full_jars),
runfiles = ctx.runfiles(p.coverage_runfiles.coverage_runfiles, transitive_files = p.runfiles.runfiles.files),
),
],
)
27 changes: 0 additions & 27 deletions scala/private/phases/phase_final.bzl

This file was deleted.

16 changes: 8 additions & 8 deletions scala/private/phases/phases.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ load(
_phase_scalatest_runfiles = "phase_scalatest_runfiles",
)
load(
"@io_bazel_rules_scala//scala/private:phases/phase_final.bzl",
_phase_binary_final = "phase_binary_final",
_phase_library_final = "phase_library_final",
_phase_scalatest_final = "phase_scalatest_final",
"@io_bazel_rules_scala//scala/private:phases/phase_default_info.bzl",
_phase_binary_default_info = "phase_binary_default_info",
_phase_library_default_info = "phase_library_default_info",
_phase_scalatest_default_info = "phase_scalatest_default_info",
)
load("@io_bazel_rules_scala//scala/private:phases/phase_scalac_provider.bzl", _phase_scalac_provider = "phase_scalac_provider")
load("@io_bazel_rules_scala//scala/private:phases/phase_write_manifest.bzl", _phase_write_manifest = "phase_write_manifest")
Expand Down Expand Up @@ -125,7 +125,7 @@ phase_library_runfiles = _phase_library_runfiles
phase_scalatest_runfiles = _phase_scalatest_runfiles
phase_common_runfiles = _phase_common_runfiles

# final
phase_binary_final = _phase_binary_final
phase_library_final = _phase_library_final
phase_scalatest_final = _phase_scalatest_final
# default_info
phase_binary_default_info = _phase_binary_default_info
phase_library_default_info = _phase_library_default_info
phase_scalatest_default_info = _phase_scalatest_default_info
5 changes: 2 additions & 3 deletions scala/private/rules/scala_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ load(
"@io_bazel_rules_scala//scala/private:phases/phases.bzl",
"extras_phases",
"phase_binary_compile",
"phase_binary_final",
"phase_binary_default_info",
"phase_common_collect_jars",
"phase_common_java_wrapper",
"phase_common_runfiles",
Expand Down Expand Up @@ -42,9 +42,8 @@ def _scala_binary_impl(ctx):
("merge_jars", phase_merge_jars),
("runfiles", phase_common_runfiles),
("write_executable", phase_common_write_executable),
("default_info", phase_binary_default_info),
],
# fixed phase
("final", phase_binary_final),
)

_scala_binary_attrs = {
Expand Down
5 changes: 2 additions & 3 deletions scala/private/rules/scala_junit_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ load("@io_bazel_rules_scala//scala/private:common_outputs.bzl", "common_outputs"
load(
"@io_bazel_rules_scala//scala/private:phases/phases.bzl",
"extras_phases",
"phase_binary_final",
"phase_binary_default_info",
"phase_common_java_wrapper",
"phase_common_runfiles",
"phase_declare_executable",
Expand Down Expand Up @@ -47,9 +47,8 @@ def _scala_junit_test_impl(ctx):
("runfiles", phase_common_runfiles),
("jvm_flags", phase_jvm_flags),
("write_executable", phase_junit_test_write_executable),
("default_info", phase_binary_default_info),
],
# fixed phase
("final", phase_binary_final),
)

_scala_junit_test_attrs = {
Expand Down
11 changes: 4 additions & 7 deletions scala/private/rules/scala_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ load(
"phase_collect_srcjars",
"phase_common_collect_jars",
"phase_library_compile",
"phase_library_final",
"phase_library_default_info",
"phase_library_for_plugin_bootstrapping_collect_jars",
"phase_library_for_plugin_bootstrapping_compile",
"phase_library_runfiles",
Expand Down Expand Up @@ -66,9 +66,8 @@ def _scala_library_impl(ctx):
("merge_jars", phase_merge_jars),
("runfiles", phase_library_runfiles),
("collect_exports_jars", phase_collect_exports_jars),
("default_info", phase_library_default_info),
],
# fixed phase
("final", phase_library_final),
)

_scala_library_attrs = {}
Expand Down Expand Up @@ -143,9 +142,8 @@ def _scala_library_for_plugin_bootstrapping_impl(ctx):
("merge_jars", phase_merge_jars),
("runfiles", phase_library_runfiles),
("collect_exports_jars", phase_collect_exports_jars),
("default_info", phase_library_default_info),
],
# fixed phase
("final", phase_library_final),
)

# the scala compiler plugin used for dependency analysis is compiled using `scala_library`.
Expand Down Expand Up @@ -199,9 +197,8 @@ def _scala_macro_library_impl(ctx):
("merge_jars", phase_merge_jars),
("runfiles", phase_library_runfiles),
("collect_exports_jars", phase_collect_exports_jars),
("default_info", phase_library_default_info),
],
# fixed phase
("final", phase_library_final),
)

_scala_macro_library_attrs = {
Expand Down
5 changes: 2 additions & 3 deletions scala/private/rules/scala_repl.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ load("@io_bazel_rules_scala//scala/private:common_outputs.bzl", "common_outputs"
load(
"@io_bazel_rules_scala//scala/private:phases/phases.bzl",
"extras_phases",
"phase_binary_final",
"phase_binary_default_info",
"phase_common_runfiles",
"phase_declare_executable",
"phase_merge_jars",
Expand Down Expand Up @@ -43,9 +43,8 @@ def _scala_repl_impl(ctx):
("merge_jars", phase_merge_jars),
("runfiles", phase_common_runfiles),
("write_executable", phase_repl_write_executable),
("default_info", phase_binary_default_info),
],
# fixed phase
("final", phase_binary_final),
)

_scala_repl_attrs = {
Expand Down
5 changes: 2 additions & 3 deletions scala/private/rules/scala_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ load(
"phase_scalac_provider",
"phase_scalatest_collect_jars",
"phase_scalatest_compile",
"phase_scalatest_final",
"phase_scalatest_default_info",
"phase_scalatest_runfiles",
"phase_scalatest_write_executable",
"phase_unused_deps_checker",
Expand All @@ -44,9 +44,8 @@ def _scala_test_impl(ctx):
("runfiles", phase_scalatest_runfiles),
("coverage_runfiles", phase_coverage_runfiles),
("write_executable", phase_scalatest_write_executable),
("default_info", phase_scalatest_default_info),
],
# fixed phase
("final", phase_scalatest_final),
)

_scala_test_attrs = {
Expand Down