diff --git a/.golangci.yml b/.golangci.yml index f338c067e2..6ec5ffdb59 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -34,6 +34,14 @@ linters: - shadow # Probably not useful enough to clean everything up - fieldalignment # TODO(peterebden): Should clean up these warnings and enable (although they are pretty finicky) enable-all: true + settings: + printf: + funcs: + # These panic with fmt.Errorf(msg, args...) under the hood, so treating them as + # printf wrappers lets vet check their format strings/args like any other Errorf call. + - (*github.com/thought-machine/please/src/parse/asp.scope).Error + - (*github.com/thought-machine/please/src/parse/asp.scope).Assert + - (*github.com/thought-machine/please/src/parse/asp.scope).NAssert perfsprint: errorf: false # Sometimes it's easier not to import another package exclusions: @@ -105,7 +113,6 @@ linters: paths: - test_data - third_party$ - - ^tree$ - ^plz-out$ - builtin$ - examples$ diff --git a/src/audit/BUILD b/src/audit/BUILD index c08aef8f93..7511020ba9 100644 --- a/src/audit/BUILD +++ b/src/audit/BUILD @@ -3,8 +3,8 @@ go_library( srcs = [ "audit.go", ], - visibility = ["PUBLIC"], pgo_file = "//:pgo", + visibility = ["PUBLIC"], deps = [ "//src/cli/logging", "//src/fs", diff --git a/src/parse/asp/interpreter.go b/src/parse/asp/interpreter.go index c9c9d3a215..f09be42feb 100644 --- a/src/parse/asp/interpreter.go +++ b/src/parse/asp/interpreter.go @@ -271,7 +271,7 @@ func (i *interpreter) Subinclude(pkgScope *scope, path string, label core.BuildL } return locals, nil }) - pkgScope.Assert(err == nil, "failed to subinclude %s: %s", label, err) + pkgScope.Assert(err == nil, "failed to subinclude %s: %w", label, err) return globals } diff --git a/src/plz/BUILD b/src/plz/BUILD index 2cc2c6690c..94beeb2960 100644 --- a/src/plz/BUILD +++ b/src/plz/BUILD @@ -1,20 +1,23 @@ go_library( name = "plz", - srcs = glob(["*.go"], exclude=["*_test.go"]), + srcs = glob( + ["*.go"], + exclude = ["*_test.go"], + ), pgo_file = "//:pgo", visibility = ["PUBLIC"], deps = [ - "///third_party/go/golang.org_x_sync//errgroup", "///third_party/go/github.com_peterebden_go-cli-init_v5//flags", + "///third_party/go/golang.org_x_sync//errgroup", "//src/build", - "//src/cmap", "//src/cli", "//src/cli/logging", + "//src/cmap", "//src/core", "//src/fs", "//src/metrics", - "//src/parse/asp", "//src/parse", + "//src/parse/asp", "//src/remote", "//src/test", ], diff --git a/test/BUILD b/test/BUILD index 1c64b97e1e..d64a42df38 100644 --- a/test/BUILD +++ b/test/BUILD @@ -66,16 +66,16 @@ gentest( plz_e2e_test( name = "test_outputs_move_test", cmd = "plz test //test:test_outputs_move_test_case -- some_test_arg", - expect_output_contains = "4 passed.", expect_file_exists = "plz-out/bin/test/foo.txt", + expect_output_contains = "4 passed.", ) gentest( name = "test_outputs_move_test_case", data = ["test_output_test_1.txt"], labels = ["manual"], - test_outputs = ["foo.txt"], test_cmd = "cp $(location test_output_test_1.txt) test.results && echo 'hello' > foo.txt #", + test_outputs = ["foo.txt"], ) # Test that on re-running a test it is cached. diff --git a/test/audit/BUILD b/test/audit/BUILD index de2551a6cc..0090d76c6c 100644 --- a/test/audit/BUILD +++ b/test/audit/BUILD @@ -2,8 +2,8 @@ subinclude("//test/build_defs") please_repo_e2e_test( name = "audit_test", - repo = "test_repo", plz_command = "./test_audit.sh", + repo = "test_repo", # Asserts that the plugin download is audited, which only happens if we actually download it. shared_cache = False, ) diff --git a/test/cyclic_dependency/BUILD b/test/cyclic_dependency/BUILD index 0232914b08..85bad0ef9f 100644 --- a/test/cyclic_dependency/BUILD +++ b/test/cyclic_dependency/BUILD @@ -2,10 +2,10 @@ subinclude("//test/build_defs") please_repo_e2e_test( name = "cyclic_dependency_test", - expected_failure = True, expect_output_contains = { "output.txt": "Dependency cycle found", }, + expected_failure = True, plz_command = "plz test //cycle:all > output.txt 2>&1", repo = "test_repo", ) diff --git a/test/get_labels/BUILD b/test/get_labels/BUILD index 5a39462645..a2d00d72f9 100644 --- a/test/get_labels/BUILD +++ b/test/get_labels/BUILD @@ -4,10 +4,10 @@ def maxdepth_test(name:str, deps:list=None, maxdepth:int, expected:list): test_case = genrule( name = name, outs = [f"{name}.sh"], + binary = True, cmd = { "opt": "echo '#!/bin/sh' > $OUTS", }, - binary = True, labels = [ f"name:{name}", "manual", @@ -41,63 +41,72 @@ def dep(name:str, deps:list=None): def echo_name_labels_up_to(maxdepth:int): def echo(name:str): - labels = get_labels(name, "name:", maxdepth=maxdepth) + labels = get_labels(name, "name:", maxdepth = maxdepth) set_command(name, "opt", " && ".join([get_command(name, "opt")] + [f"echo 'echo {l}' >> $OUTS" for l in labels])) + return echo -dep(name = "dep1", deps = [":dep3"]) +dep( + name = "dep1", + deps = [":dep3"], +) + dep(name = "dep2") -dep(name = "dep3", deps = [":dep4", ":dep5"]) + +dep( + name = "dep3", + deps = [ + ":dep4", + ":dep5", + ], +) + dep(name = "dep4") + dep(name = "dep5") maxdepth_test( name = "target_only", + expected = ["target_only"], + maxdepth = 0, deps = [ ":dep1", ":dep2", ], - maxdepth = 0, - expected = ["target_only"], ) maxdepth_test( name = "direct_deps", - deps = [ - ":dep1", - ":dep2", - ], - maxdepth = 1, expected = [ "dep1", "dep2", "direct_deps", ], -) - -maxdepth_test( - name = "second_level_deps", + maxdepth = 1, deps = [ ":dep1", ":dep2", - ":dep3", ], - maxdepth = 2, +) + +maxdepth_test( + name = "second_level_deps", expected = [ "dep1", "dep2", "dep3", "second_level_deps", ], -) - -maxdepth_test( - name = "all_deps", + maxdepth = 2, deps = [ ":dep1", ":dep2", + ":dep3", ], - maxdepth = -1, +) + +maxdepth_test( + name = "all_deps", expected = [ "all_deps", "dep1", @@ -106,6 +115,11 @@ maxdepth_test( "dep4", "dep5", ], + maxdepth = -1, + deps = [ + ":dep1", + ":dep2", + ], ) # Regression test: a pre-build function calling get_labels() must see the target's whole transitive diff --git a/test/parse/subinclude_race/BUILD b/test/parse/subinclude_race/BUILD index 7027363a65..d878d1fdbe 100644 --- a/test/parse/subinclude_race/BUILD +++ b/test/parse/subinclude_race/BUILD @@ -2,12 +2,6 @@ subinclude("//test/build_defs") please_repo_e2e_test( name = "subinclude_race_test", - expected_output = { - "plz-out/gen/a/x.txt": "hello", - "plz-out/gen/b/y.txt": "hello", - }, - plz_command = "plz build //a:x //b:y", - repo = "test_repo", # Regression test for a deadlock between two packages that each subinclude their own output, where # that cascades into needing a target defined by the other package (a:self -> b:self -> b:dep -> # a:dep). Depending on which package's parse gets claimed first, the goroutine parsing one can end @@ -17,4 +11,10 @@ please_repo_e2e_test( # able to catch it - only requesting both packages independently, in parallel, reproduces it. # Bounded so a regression fails the test rather than hanging until the CI job is killed. timeout = 60, + expected_output = { + "plz-out/gen/a/x.txt": "hello", + "plz-out/gen/b/y.txt": "hello", + }, + plz_command = "plz build //a:x //b:y", + repo = "test_repo", ) diff --git a/test/runtime_deps/BUILD b/test/runtime_deps/BUILD index 8f07efff64..6fbf840ea1 100644 --- a/test/runtime_deps/BUILD +++ b/test/runtime_deps/BUILD @@ -13,14 +13,14 @@ please_repo_e2e_test( # print the target's direct run-time dependencies. please_repo_e2e_test( name = "query_print_test", - plz_command = " && ".join([ - "plz query print -f runtime_deps //test:runtime_deps_test_case > runtime_deps_test_case", - "plz query print -f runtime_deps //test:target_with_no_runtime_deps > target_with_no_runtime_deps", - ]), expected_output = { "runtime_deps_test_case": "//test:target_with_runtime_deps", "target_with_no_runtime_deps": "", }, + plz_command = " && ".join([ + "plz query print -f runtime_deps //test:runtime_deps_test_case > runtime_deps_test_case", + "plz query print -f runtime_deps //test:target_with_no_runtime_deps > target_with_no_runtime_deps", + ]), repo = "repo", ) @@ -50,20 +50,20 @@ _expected_deps = """\ please_repo_e2e_test( name = "query_deps_test", - plz_command = "plz query deps //test:runtime_deps_test_case > deps", expected_output = { "deps": _expected_deps, }, + plz_command = "plz query deps //test:runtime_deps_test_case > deps", repo = "repo", ) # Ensure that run-time dependencies are in fact considered dependencies by `plz query revdeps`. please_repo_e2e_test( name = "query_revdeps_test", - plz_command = "plz query revdeps //test:another_runtime_dep > revdeps", expected_output = { "revdeps": "//test:target_with_another_runtime_dep", }, + plz_command = "plz query revdeps //test:another_runtime_dep > revdeps", repo = "repo", ) diff --git a/test/source_list_files/BUILD b/test/source_list_files/BUILD index b6d95ab7e9..95b835bf56 100644 --- a/test/source_list_files/BUILD +++ b/test/source_list_files/BUILD @@ -2,34 +2,34 @@ subinclude("//test/build_defs") please_repo_e2e_test( name = "srcs_test", - plz_command = "plz build //:with_srcs", expected_output = { - "plz-out/gen/with_srcs.txt": "a.txt\nb.txt", + "plz-out/gen/with_srcs.txt": "a.txt\nb.txt", }, + plz_command = "plz build //:with_srcs", repo = "test_repo", ) please_repo_e2e_test( name = "src_test", - plz_command = "plz build //:with_one_src", expected_output = { - "plz-out/gen/with_one_src.txt": "a.txt", + "plz-out/gen/with_one_src.txt": "a.txt", }, + plz_command = "plz build //:with_one_src", repo = "test_repo", ) please_repo_e2e_test( name = "named_srcs_test", - plz_command = "plz build //:with_named_srcs", expected_output = { - "plz-out/gen/with_named_srcs.txt": "a.txt\nb.txt", + "plz-out/gen/with_named_srcs.txt": "a.txt\nb.txt", }, + plz_command = "plz build //:with_named_srcs", repo = "test_repo", ) please_repo_e2e_test( name = "flag_not_set_test", + expected_failure = True, plz_command = "plz build //:with_flag_not_set", repo = "test_repo", - expected_failure = True, ) diff --git a/test/subrepo/parse_deadlock/BUILD b/test/subrepo/parse_deadlock/BUILD index 2d06ce4089..4ddb97ae98 100644 --- a/test/subrepo/parse_deadlock/BUILD +++ b/test/subrepo/parse_deadlock/BUILD @@ -2,11 +2,11 @@ subinclude("//test/build_defs") please_repo_e2e_test( name = "parse_deadlock_test", + # Bounded so a regression fails the test rather than hanging until the CI job is killed. + timeout = 120, expected_output = { "plz-out/gen/pkg/x.txt": "hello", }, plz_command = "plz build //pkg:x", repo = "test_repo", - # Bounded so a regression fails the test rather than hanging until the CI job is killed. - timeout = 120, )