Skip to content

Commit

Permalink
[bazel] cc_binary_with_flags rule: Fix runfile generation.
Browse files Browse the repository at this point in the history
Currently, the cc_binary_with_flags rule fails to generate a runfiles directory with the resources provided via the "data" attribute. This is because the DefaultInfo provider returned by the transition_rule rule is passed the cc_binary's runfiles via the "data_runfiles" constructor argument, rather than a "runfiles" constructor argument, and because the runfiles are pulled from the "data_runfiles" attribute of the cc_binary's DefaultInfo provider, rather than from the "default_runfiles" attribute. The aforementioned DefaultInfo constructor parameter and attribute are discouraged, see https://bazel.build/rules/lib/DefaultInfo and https://bazel.build/extending/rules#runfiles_features_to_avoid. The fix is to use the DefaultInfo constructor parameter and attribute recommended in said links.

Note that the cc_test_with_flags rule does not have this problem because it already uses the appropriate DefaultInfo constructor argument and attribute: https://skia.googlesource.com/skia/+/f4803c264c541431d71f0c89e57904b5e059f78e/bazel/cc_test_with_flags.bzl#27.

The remainder of this CL description describes how I tested this change.

On patchset 1, I turned a cc_test_with_flags into a cc_binary_with_flags to illustrate the issue.

    $ bazel build //tests:ImageBitmapTest --config=for_linux_x64_with_rbe
    $ ls bazel-bin/tests
    ImageBitmapTest

Note that only the binary was generated. There is no runfiles directory.

On patchset 2, I modified the cc_binary_with_flags rule with the proposed change.

    $ bazel build //tests:ImageBitmapTest --config=for_linux_x64_with_rbe
    $ ls bazel-bin/tests
    ImageBitmapTest
    ImageBitmapTest.runfiles
    ImageBitmapTest.runfiles_manifest

Note that this time the runfiles directory was generated. We can confirm that it pulled the expected resources by inspecting said directory:

    $ find bazel-bin/tests/ImageBitmapTest.runfiles
    bazel-bin/tests/ImageBitmapTest.runfiles/
    bazel-bin/tests/ImageBitmapTest.runfiles/skia
    bazel-bin/tests/ImageBitmapTest.runfiles/skia/resources
    bazel-bin/tests/ImageBitmapTest.runfiles/skia/resources/pdf_command_stream.txt
    bazel-bin/tests/ImageBitmapTest.runfiles/skia/resources/text
    bazel-bin/tests/ImageBitmapTest.runfiles/skia/resources/text/han_traditional.txt
    bazel-bin/tests/ImageBitmapTest.runfiles/skia/resources/text/sundanese.txt
    bazel-bin/tests/ImageBitmapTest.runfiles/skia/resources/text/greek.txt
    ...

On patchset 3, I undid the changes from patchset 1 to get the CL ready for review.

Bug: skia:14227
Change-Id: I84eee76f0c6c8efc503beeb6901e405c66794b06
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/667644
Commit-Queue: Leandro Lovisolo <[email protected]>
Reviewed-by: Kevin Lubick <[email protected]>
  • Loading branch information
LeandroLovisolo authored and SkCQ committed Apr 10, 2023
1 parent 7eebce4 commit cde087b
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion bazel/cc_binary_with_flags.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def _transition_rule_impl(ctx):
return [
DefaultInfo(
executable = outfile,
data_runfiles = actual_binary[DefaultInfo].data_runfiles,
runfiles = actual_binary[DefaultInfo].default_runfiles,
),
]

Expand Down

0 comments on commit cde087b

Please sign in to comment.