From b4628eb03fe28b3f1f1a97bb0d8d9a1944eb001d Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Thu, 8 Apr 2021 14:40:57 +0000 Subject: [PATCH] bp2build: codegen singleton lists on one line. This CL refactors the bp2build code generator to pretty print lists with a single element on one line, instead of taking up three lines, which can make BUILD files unnecessarily long. A single line singleton list is also more commonly used in BUILD files. Test: TH Change-Id: Ic9e44741bbb070c8f45925466b9ccdd0608498b2 --- bp2build/build_conversion.go | 31 +++- bp2build/build_conversion_test.go | 156 +++++------------- bp2build/cc_library_conversion_test.go | 64 ++----- .../cc_library_headers_conversion_test.go | 60 ++----- bp2build/cc_library_static_conversion_test.go | 16 +- bp2build/cc_object_conversion_test.go | 140 ++++------------ bp2build/python_binary_conversion_test.go | 25 +-- bp2build/sh_conversion_test.go | 4 +- 8 files changed, 142 insertions(+), 354 deletions(-) diff --git a/bp2build/build_conversion.go b/bp2build/build_conversion.go index 16e7278da..b7a28100b 100644 --- a/bp2build/build_conversion.go +++ b/bp2build/build_conversion.go @@ -394,21 +394,34 @@ func prettyPrint(propertyValue reflect.Value, indent int) (string, error) { return "", nil } - ret = "[\n" - for i := 0; i < propertyValue.Len(); i++ { - indexedValue, err := prettyPrint(propertyValue.Index(i), indent+1) + if propertyValue.Len() == 1 { + // Single-line list for list with only 1 element + ret += "[" + indexedValue, err := prettyPrint(propertyValue.Index(0), indent) if err != nil { return "", err } + ret += indexedValue + ret += "]" + } else { + // otherwise, use a multiline list. + ret += "[\n" + for i := 0; i < propertyValue.Len(); i++ { + indexedValue, err := prettyPrint(propertyValue.Index(i), indent+1) + if err != nil { + return "", err + } - if indexedValue != "" { - ret += makeIndent(indent + 1) - ret += indexedValue - ret += ",\n" + if indexedValue != "" { + ret += makeIndent(indent + 1) + ret += indexedValue + ret += ",\n" + } } + ret += makeIndent(indent) + ret += "]" } - ret += makeIndent(indent) - ret += "]" + case reflect.Struct: // Special cases where the bp2build sends additional information to the codegenerator // by wrapping the attributes in a custom struct type. diff --git a/bp2build/build_conversion_test.go b/bp2build/build_conversion_test.go index 49897b3ca..1ede4428a 100644 --- a/bp2build/build_conversion_test.go +++ b/bp2build/build_conversion_test.go @@ -27,9 +27,7 @@ func TestGenerateSoongModuleTargets(t *testing.T) { expectedBazelTarget string }{ { - bp: `custom { - name: "foo", -} + bp: `custom { name: "foo" } `, expectedBazelTarget: `soong_module( name = "foo", @@ -85,9 +83,7 @@ func TestGenerateSoongModuleTargets(t *testing.T) { soong_module_variant = "", soong_module_deps = [ ], - required = [ - "bar", - ], + required = ["bar"], )`, }, { @@ -116,12 +112,10 @@ func TestGenerateSoongModuleTargets(t *testing.T) { targets: ["goal_foo"], tag: ".foo", }, - dists: [ - { - targets: ["goal_bar"], - tag: ".bar", - }, - ], + dists: [{ + targets: ["goal_bar"], + tag: ".bar", + }], } `, expectedBazelTarget: `soong_module( @@ -133,18 +127,12 @@ func TestGenerateSoongModuleTargets(t *testing.T) { ], dist = { "tag": ".foo", - "targets": [ - "goal_foo", - ], + "targets": ["goal_foo"], }, - dists = [ - { - "tag": ".bar", - "targets": [ - "goal_bar", - ], - }, - ], + dists = [{ + "tag": ".bar", + "targets": ["goal_bar"], + }], )`, }, { @@ -169,19 +157,13 @@ func TestGenerateSoongModuleTargets(t *testing.T) { soong_module_variant = "", soong_module_deps = [ ], - dists = [ - { - "tag": ".tag", - "targets": [ - "my_goal", - ], - }, - ], + dists = [{ + "tag": ".tag", + "targets": ["my_goal"], + }], owner = "custom_owner", ramdisk = True, - required = [ - "bar", - ], + required = ["bar"], target_required = [ "qux", "bazqux", @@ -553,9 +535,7 @@ genrule { }`, expectedBazelTargets: []string{`filegroup( name = "fg_foo", - srcs = [ - "b", - ], + srcs = ["b"], )`, }, }, @@ -625,7 +605,7 @@ genrule { bp: `filegroup { name: "foobar", srcs: [ - ":foo", + ":foo", "c", ], bazel_module: { bp2build_available: true }, @@ -671,25 +651,15 @@ genrule { `genrule( name = "foo", cmd = "$(location :foo.tool) --genDir=$(GENDIR) arg $(SRCS) $(OUTS)", - outs = [ - "foo.out", - ], - srcs = [ - "foo.in", - ], - tools = [ - ":foo.tool", - ], + outs = ["foo.out"], + srcs = ["foo.in"], + tools = [":foo.tool"], )`, `genrule( name = "foo.tool", cmd = "cp $(SRCS) $(OUTS)", - outs = [ - "foo_tool.out", - ], - srcs = [ - "foo_tool.in", - ], + outs = ["foo_tool.out"], + srcs = ["foo_tool.in"], )`, }, }, @@ -718,15 +688,9 @@ genrule { expectedBazelTargets: []string{`genrule( name = "foo", cmd = "$(locations :foo.tools) -s $(OUTS) $(SRCS)", - outs = [ - "foo.out", - ], - srcs = [ - "foo.in", - ], - tools = [ - ":foo.tools", - ], + outs = ["foo.out"], + srcs = ["foo.in"], + tools = [":foo.tools"], )`, `genrule( name = "foo.tools", @@ -735,9 +699,7 @@ genrule { "foo_tool.out", "foo_tool2.out", ], - srcs = [ - "foo_tool.in", - ], + srcs = ["foo_tool.in"], )`, }, }, @@ -758,15 +720,9 @@ genrule { expectedBazelTargets: []string{`genrule( name = "foo", cmd = "$(locations //other:foo.tool) -s $(OUTS) $(SRCS)", - outs = [ - "foo.out", - ], - srcs = [ - "foo.in", - ], - tools = [ - "//other:foo.tool", - ], + outs = ["foo.out"], + srcs = ["foo.in"], + tools = ["//other:foo.tool"], )`, }, fs: otherGenruleBp, @@ -788,15 +744,9 @@ genrule { expectedBazelTargets: []string{`genrule( name = "foo", cmd = "$(locations //other:foo.tool) -s $(OUTS) $(location //other:other.tool)", - outs = [ - "foo.out", - ], - srcs = [ - "//other:other.tool", - ], - tools = [ - "//other:foo.tool", - ], + outs = ["foo.out"], + srcs = ["//other:other.tool"], + tools = ["//other:foo.tool"], )`, }, fs: otherGenruleBp, @@ -818,12 +768,8 @@ genrule { expectedBazelTargets: []string{`genrule( name = "foo", cmd = "$(location //other:foo.tool) -s $(OUTS) $(SRCS)", - outs = [ - "foo.out", - ], - srcs = [ - "foo.in", - ], + outs = ["foo.out"], + srcs = ["foo.in"], tools = [ "//other:foo.tool", "//other:other.tool", @@ -849,12 +795,8 @@ genrule { expectedBazelTargets: []string{`genrule( name = "foo", cmd = "$(locations //other:foo.tool) -s $(OUTS) $(SRCS)", - outs = [ - "foo.out", - ], - srcs = [ - "foo.in", - ], + outs = ["foo.out"], + srcs = ["foo.in"], tools = [ "//other:foo.tool", "//other:other.tool", @@ -879,12 +821,8 @@ genrule { expectedBazelTargets: []string{`genrule( name = "foo", cmd = "cp $(SRCS) $(OUTS)", - outs = [ - "foo.out", - ], - srcs = [ - "foo.in", - ], + outs = ["foo.out"], + srcs = ["foo.in"], )`, }, }, @@ -988,12 +926,8 @@ genrule { expectedBazelTarget: `genrule( name = "gen", cmd = "do-something $(SRCS) $(OUTS)", - outs = [ - "out", - ], - srcs = [ - "in1", - ], + outs = ["out"], + srcs = ["in1"], )`, description: "genrule applies properties from a genrule_defaults dependency if not specified", }, @@ -1062,12 +996,8 @@ genrule { expectedBazelTarget: `genrule( name = "gen", cmd = "cp $(SRCS) $(OUTS)", - outs = [ - "out", - ], - srcs = [ - "in1", - ], + outs = ["out"], + srcs = ["in1"], )`, description: "genrule applies properties from list of genrule_defaults", }, diff --git a/bp2build/cc_library_conversion_test.go b/bp2build/cc_library_conversion_test.go index 6a148a8a3..783af2e7c 100644 --- a/bp2build/cc_library_conversion_test.go +++ b/bp2build/cc_library_conversion_test.go @@ -109,12 +109,8 @@ cc_library { `, expectedBazelTargets: []string{`cc_library( name = "foo-lib", - copts = [ - "-Wall", - ], - deps = [ - ":some-headers", - ], + copts = ["-Wall"], + deps = [":some-headers"], hdrs = [ "header.h", "header.hh", @@ -127,40 +123,20 @@ cc_library { "header.h.generic", "foo-dir/a.h", ], - includes = [ - "foo-dir", - ], - linkopts = [ - "-Wl,--exclude-libs=bar.a", - ] + select({ - "//build/bazel/platforms/arch:x86": [ - "-Wl,--exclude-libs=baz.a", - ], - "//build/bazel/platforms/arch:x86_64": [ - "-Wl,--exclude-libs=qux.a", - ], + includes = ["foo-dir"], + linkopts = ["-Wl,--exclude-libs=bar.a"] + select({ + "//build/bazel/platforms/arch:x86": ["-Wl,--exclude-libs=baz.a"], + "//build/bazel/platforms/arch:x86_64": ["-Wl,--exclude-libs=qux.a"], "//conditions:default": [], }), - srcs = [ - "impl.cpp", - ] + select({ - "//build/bazel/platforms/arch:x86": [ - "x86.cpp", - ], - "//build/bazel/platforms/arch:x86_64": [ - "x86_64.cpp", - ], + srcs = ["impl.cpp"] + select({ + "//build/bazel/platforms/arch:x86": ["x86.cpp"], + "//build/bazel/platforms/arch:x86_64": ["x86_64.cpp"], "//conditions:default": [], }) + select({ - "//build/bazel/platforms/os:android": [ - "android.cpp", - ], - "//build/bazel/platforms/os:darwin": [ - "darwin.cpp", - ], - "//build/bazel/platforms/os:linux": [ - "linux.cpp", - ], + "//build/bazel/platforms/os:android": ["android.cpp"], + "//build/bazel/platforms/os:darwin": ["darwin.cpp"], + "//build/bazel/platforms/os:linux": ["linux.cpp"], "//conditions:default": [], }), )`}, @@ -215,9 +191,7 @@ cc_library { "-Wunused", "-Werror", ], - deps = [ - ":libc_headers", - ], + deps = [":libc_headers"], hdrs = [ "linked_list.h", "linker.h", @@ -232,17 +206,11 @@ cc_library { "-Wl,--exclude-libs=libclang_rt.builtins-i686-android.a", "-Wl,--exclude-libs=libclang_rt.builtins-x86_64-android.a", ] + select({ - "//build/bazel/platforms/arch:x86": [ - "-Wl,--exclude-libs=libgcc_eh.a", - ], - "//build/bazel/platforms/arch:x86_64": [ - "-Wl,--exclude-libs=libgcc_eh.a", - ], + "//build/bazel/platforms/arch:x86": ["-Wl,--exclude-libs=libgcc_eh.a"], + "//build/bazel/platforms/arch:x86_64": ["-Wl,--exclude-libs=libgcc_eh.a"], "//conditions:default": [], }), - srcs = [ - "ld_android.cpp", - ], + srcs = ["ld_android.cpp"], )`}, }, } diff --git a/bp2build/cc_library_headers_conversion_test.go b/bp2build/cc_library_headers_conversion_test.go index 655218d29..c59241f2c 100644 --- a/bp2build/cc_library_headers_conversion_test.go +++ b/bp2build/cc_library_headers_conversion_test.go @@ -141,30 +141,18 @@ cc_library_headers { "dir-2/dir2a.h", "dir-2/dir2b.h", ] + select({ - "//build/bazel/platforms/arch:arm64": [ - "arch_arm64_exported_include_dir/a.h", - ], - "//build/bazel/platforms/arch:x86": [ - "arch_x86_exported_include_dir/b.h", - ], - "//build/bazel/platforms/arch:x86_64": [ - "arch_x86_64_exported_include_dir/c.h", - ], + "//build/bazel/platforms/arch:arm64": ["arch_arm64_exported_include_dir/a.h"], + "//build/bazel/platforms/arch:x86": ["arch_x86_exported_include_dir/b.h"], + "//build/bazel/platforms/arch:x86_64": ["arch_x86_64_exported_include_dir/c.h"], "//conditions:default": [], }), includes = [ "dir-1", "dir-2", ] + select({ - "//build/bazel/platforms/arch:arm64": [ - "arch_arm64_exported_include_dir", - ], - "//build/bazel/platforms/arch:x86": [ - "arch_x86_exported_include_dir", - ], - "//build/bazel/platforms/arch:x86_64": [ - "arch_x86_64_exported_include_dir", - ], + "//build/bazel/platforms/arch:arm64": ["arch_arm64_exported_include_dir"], + "//build/bazel/platforms/arch:x86": ["arch_x86_exported_include_dir"], + "//build/bazel/platforms/arch:x86_64": ["arch_x86_64_exported_include_dir"], "//conditions:default": [], }), )`, `cc_library_headers( @@ -173,18 +161,14 @@ cc_library_headers { "lib-1/lib1a.h", "lib-1/lib1b.h", ], - includes = [ - "lib-1", - ], + includes = ["lib-1"], )`, `cc_library_headers( name = "lib-2", hdrs = [ "lib-2/lib2a.h", "lib-2/lib2b.h", ], - includes = [ - "lib-2", - ], + includes = ["lib-2"], )`}, }, { @@ -223,27 +207,13 @@ cc_library_headers { name = "darwin-lib", )`, `cc_library_headers( name = "foo_headers", - deps = [ - ":base-lib", - ] + select({ - "//build/bazel/platforms/os:android": [ - ":android-lib", - ], - "//build/bazel/platforms/os:darwin": [ - ":darwin-lib", - ], - "//build/bazel/platforms/os:fuchsia": [ - ":fuchsia-lib", - ], - "//build/bazel/platforms/os:linux": [ - ":linux-lib", - ], - "//build/bazel/platforms/os:linux_bionic": [ - ":linux_bionic-lib", - ], - "//build/bazel/platforms/os:windows": [ - ":windows-lib", - ], + deps = [":base-lib"] + select({ + "//build/bazel/platforms/os:android": [":android-lib"], + "//build/bazel/platforms/os:darwin": [":darwin-lib"], + "//build/bazel/platforms/os:fuchsia": [":fuchsia-lib"], + "//build/bazel/platforms/os:linux": [":linux-lib"], + "//build/bazel/platforms/os:linux_bionic": [":linux_bionic-lib"], + "//build/bazel/platforms/os:windows": [":windows-lib"], "//conditions:default": [], }), )`, `cc_library_headers( diff --git a/bp2build/cc_library_static_conversion_test.go b/bp2build/cc_library_static_conversion_test.go index a6a002818..427aed355 100644 --- a/bp2build/cc_library_static_conversion_test.go +++ b/bp2build/cc_library_static_conversion_test.go @@ -231,9 +231,7 @@ cc_library_static { "implicit_include_1.h", "implicit_include_2.h", ], - includes = [ - ".", - ], + includes = ["."], linkstatic = True, srcs = [ "static_lib_1.cc", @@ -246,9 +244,7 @@ cc_library_static { "implicit_include_1.h", "implicit_include_2.h", ], - includes = [ - ".", - ], + includes = ["."], linkstatic = True, srcs = [ "static_lib_2.cc", @@ -261,9 +257,7 @@ cc_library_static { "implicit_include_1.h", "implicit_include_2.h", ], - includes = [ - ".", - ], + includes = ["."], linkstatic = True, srcs = [ "whole_static_lib_1.cc", @@ -276,9 +270,7 @@ cc_library_static { "implicit_include_1.h", "implicit_include_2.h", ], - includes = [ - ".", - ], + includes = ["."], linkstatic = True, srcs = [ "whole_static_lib_2.cc", diff --git a/bp2build/cc_object_conversion_test.go b/bp2build/cc_object_conversion_test.go index 840bf4299..a9d24ace0 100644 --- a/bp2build/cc_object_conversion_test.go +++ b/bp2build/cc_object_conversion_test.go @@ -75,9 +75,7 @@ func TestCcObjectBp2Build(t *testing.T) { "include", ".", ], - srcs = [ - "a/b/c.c", - ], + srcs = ["a/b/c.c"], )`, }, }, @@ -124,9 +122,7 @@ cc_defaults { "include", ".", ], - srcs = [ - "a/b/c.c", - ], + srcs = ["a/b/c.c"], )`, }, }, @@ -156,29 +152,15 @@ cc_object { `, expectedBazelTargets: []string{`cc_object( name = "bar", - copts = [ - "-fno-addrsig", - ], - local_include_dirs = [ - ".", - ], - srcs = [ - "x/y/z.c", - ], + copts = ["-fno-addrsig"], + local_include_dirs = ["."], + srcs = ["x/y/z.c"], )`, `cc_object( name = "foo", - copts = [ - "-fno-addrsig", - ], - deps = [ - ":bar", - ], - local_include_dirs = [ - ".", - ], - srcs = [ - "a/b/c.c", - ], + copts = ["-fno-addrsig"], + deps = [":bar"], + local_include_dirs = ["."], + srcs = ["a/b/c.c"], )`, }, }, @@ -201,12 +183,8 @@ cc_object { `, expectedBazelTargets: []string{`cc_object( name = "foo", - copts = [ - "-fno-addrsig", - ], - srcs = [ - "a/b/c.c", - ], + copts = ["-fno-addrsig"], + srcs = ["a/b/c.c"], )`, }, }, @@ -229,12 +207,8 @@ cc_object { `, expectedBazelTargets: []string{`cc_object( name = "foo", - asflags = [ - "-DPLATFORM_SDK_VERSION={Platform_sdk_version}", - ], - copts = [ - "-fno-addrsig", - ], + asflags = ["-DPLATFORM_SDK_VERSION={Platform_sdk_version}"], + copts = ["-fno-addrsig"], )`, }, }, @@ -322,23 +296,13 @@ func TestCcObjectConfigurableAttributesBp2Build(t *testing.T) { expectedBazelTargets: []string{ `cc_object( name = "foo", - copts = [ - "-fno-addrsig", - ] + select({ - "//build/bazel/platforms/arch:x86": [ - "-fPIC", - ], + copts = ["-fno-addrsig"] + select({ + "//build/bazel/platforms/arch:x86": ["-fPIC"], "//conditions:default": [], }), - local_include_dirs = [ - ".", - ], - srcs = [ - "a.cpp", - ] + select({ - "//build/bazel/platforms/arch:arm": [ - "arch/arm/file.S", - ], + local_include_dirs = ["."], + srcs = ["a.cpp"] + select({ + "//build/bazel/platforms/arch:arm": ["arch/arm/file.S"], "//conditions:default": [], }), )`, @@ -376,41 +340,19 @@ func TestCcObjectConfigurableAttributesBp2Build(t *testing.T) { expectedBazelTargets: []string{ `cc_object( name = "foo", - copts = [ - "-fno-addrsig", - ] + select({ - "//build/bazel/platforms/arch:arm": [ - "-Wall", - ], - "//build/bazel/platforms/arch:arm64": [ - "-Wall", - ], - "//build/bazel/platforms/arch:x86": [ - "-fPIC", - ], - "//build/bazel/platforms/arch:x86_64": [ - "-fPIC", - ], + copts = ["-fno-addrsig"] + select({ + "//build/bazel/platforms/arch:arm": ["-Wall"], + "//build/bazel/platforms/arch:arm64": ["-Wall"], + "//build/bazel/platforms/arch:x86": ["-fPIC"], + "//build/bazel/platforms/arch:x86_64": ["-fPIC"], "//conditions:default": [], }), - local_include_dirs = [ - ".", - ], - srcs = [ - "base.cpp", - ] + select({ - "//build/bazel/platforms/arch:arm": [ - "arm.cpp", - ], - "//build/bazel/platforms/arch:arm64": [ - "arm64.cpp", - ], - "//build/bazel/platforms/arch:x86": [ - "x86.cpp", - ], - "//build/bazel/platforms/arch:x86_64": [ - "x86_64.cpp", - ], + local_include_dirs = ["."], + srcs = ["base.cpp"] + select({ + "//build/bazel/platforms/arch:arm": ["arm.cpp"], + "//build/bazel/platforms/arch:arm64": ["arm64.cpp"], + "//build/bazel/platforms/arch:x86": ["x86.cpp"], + "//build/bazel/platforms/arch:x86_64": ["x86_64.cpp"], "//conditions:default": [], }), )`, @@ -441,26 +383,14 @@ func TestCcObjectConfigurableAttributesBp2Build(t *testing.T) { expectedBazelTargets: []string{ `cc_object( name = "foo", - copts = [ - "-fno-addrsig", - ] + select({ - "//build/bazel/platforms/os:android": [ - "-fPIC", - ], - "//build/bazel/platforms/os:darwin": [ - "-Wall", - ], - "//build/bazel/platforms/os:windows": [ - "-fPIC", - ], + copts = ["-fno-addrsig"] + select({ + "//build/bazel/platforms/os:android": ["-fPIC"], + "//build/bazel/platforms/os:darwin": ["-Wall"], + "//build/bazel/platforms/os:windows": ["-fPIC"], "//conditions:default": [], }), - local_include_dirs = [ - ".", - ], - srcs = [ - "base.cpp", - ], + local_include_dirs = ["."], + srcs = ["base.cpp"], )`, }, }, diff --git a/bp2build/python_binary_conversion_test.go b/bp2build/python_binary_conversion_test.go index 7600e3651..2054e0678 100644 --- a/bp2build/python_binary_conversion_test.go +++ b/bp2build/python_binary_conversion_test.go @@ -33,24 +33,15 @@ func TestPythonBinaryHost(t *testing.T) { blueprint: `python_binary_host { name: "foo", main: "a.py", - srcs: [ - "**/*.py" - ], - exclude_srcs: [ - "b/e.py" - ], - data: [ - "files/data.txt", - ], - + srcs: ["**/*.py"], + exclude_srcs: ["b/e.py"], + data: ["files/data.txt",], bazel_module: { bp2build_available: true }, } `, expectedBazelTargets: []string{`py_binary( name = "foo", - data = [ - "files/data.txt", - ], + data = ["files/data.txt"], main = "a.py", srcs = [ "a.py", @@ -83,9 +74,7 @@ func TestPythonBinaryHost(t *testing.T) { expectedBazelTargets: []string{`py_binary( name = "foo", python_version = "PY2", - srcs = [ - "a.py", - ], + srcs = ["a.py"], )`, }, }, @@ -113,9 +102,7 @@ func TestPythonBinaryHost(t *testing.T) { // python_version is PY3 by default. `py_binary( name = "foo", - srcs = [ - "a.py", - ], + srcs = ["a.py"], )`, }, }, diff --git a/bp2build/sh_conversion_test.go b/bp2build/sh_conversion_test.go index 2aa373c3f..37f542ef7 100644 --- a/bp2build/sh_conversion_test.go +++ b/bp2build/sh_conversion_test.go @@ -74,9 +74,7 @@ func TestShBinaryBp2Build(t *testing.T) { }`, expectedBazelTargets: []string{`sh_binary( name = "foo", - srcs = [ - "foo.sh", - ], + srcs = ["foo.sh"], )`}, }, }