From 42b1cd9c488b2dcf2acac57846421cf717cae49f Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Tue, 20 Jun 2023 16:38:35 +0100 Subject: [PATCH 01/27] This doesn't work (#131) --- tools/please_go/generate/generate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/please_go/generate/generate.go b/tools/please_go/generate/generate.go index 0c381b9d..77d37689 100644 --- a/tools/please_go/generate/generate.go +++ b/tools/please_go/generate/generate.go @@ -406,8 +406,8 @@ func nameForLibInPkg(module, pkg string) string { // trimPath is like strings.TrimPrefix but is path aware. It removes base from target if target starts with base, // otherwise returns target unmodified. func trimPath(target, base string) string { - baseParts := filepath.SplitList(base) - targetParts := filepath.SplitList(target) + baseParts := strings.Split(filepath.Clean(base), "/") + targetParts := strings.Split(filepath.Clean(target), "/") if len(targetParts) < len(baseParts) { return target From 61ebb86dec780577ad3c8b33a09f3985fdee3d3d Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Fri, 23 Jun 2023 15:06:56 +0100 Subject: [PATCH 02/27] Tag v1.7.2 (#133) --- ChangeLog | 5 +++++ VERSION | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 842148b5..33f3cf4a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +Version 1.7.2 +------------ + * Use correct version of the go tool (#128) + * Write `asm.h` to the tmp dir rather than the goroot (#125) + Version 1.7.1 ------------ * Added missing `resources` option to `go_benchmark` (#121) diff --git a/VERSION b/VERSION index 943f9cbc..f8a696c8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.1 +1.7.2 From b7694463c9804507f64964db8e3cc0dbb2ec428b Mon Sep 17 00:00:00 2001 From: Chris Novakovic Date: Mon, 26 Jun 2023 15:18:39 +0100 Subject: [PATCH 03/27] Refactor `please_go` download test to avoid subrepo references (#136) * Refactor `please_go` download test to avoid subrepo references The architecture-specific subrepo references in `//tools:download_test` aren't meaningful outside the go-rules repo, causing `plz query graph` to fail with the following error in any parent repo that includes go-rules: ``` CRITICAL: Target ///darwin_amd64//tools:please_go not found in build graph ``` Refactor the downloading of `please_go` and the download test to avoid making subrepo references that might not exist in other contexts. Fixes #135. * Remove intermediate genrule --- tools/BUILD | 42 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/tools/BUILD b/tools/BUILD index 0d8c9cc5..e7474792 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -1,28 +1,20 @@ VERSION = "1.4.1" -remote_file( - name = "please_go", - binary = True, - hashes = [ - "39ba7a7478c16306c7758e81aa53d7ff2908894a8fbc31b1809c2ede7950911e", # please_go-1.4.1-darwin_amd64 - "342a23306fca55ec7c3c56ccc96af8e61131897c5ba9c245d2d6c4bc293ee1fd", # please_go-1.4.1-darwin_arm64 - "5273e338aa07f2f2b1a68a08a14acebee52f8a9758f209b0c9deb7cc0a3b90f4", # please_go-1.4.1-freebsd_amd64 - "beb961aec753e4e9b819e8ab259c30c019229903e27f51b5095322b8c9c0ada7", # please_go-1.4.1-linux_amd64 - "1644712fa35b076b7d4b65e4e250fe9a2c01516561841aa362568f3a26f4f0d2", # please_go-1.4.1-linux_arm64 - ], - labels = ["manual"], # Needed to avoid picking this up for coverage - url = f"https://github.com/please-build/go-rules/releases/download/please-go-v{VERSION}/please_go-{VERSION}-{CONFIG.OS}_{CONFIG.ARCH}", - visibility = ["PUBLIC"], -) +hashes = { + "darwin_amd64": "39ba7a7478c16306c7758e81aa53d7ff2908894a8fbc31b1809c2ede7950911e", + "darwin_arm64": "342a23306fca55ec7c3c56ccc96af8e61131897c5ba9c245d2d6c4bc293ee1fd", + "freebsd_amd64": "5273e338aa07f2f2b1a68a08a14acebee52f8a9758f209b0c9deb7cc0a3b90f4", + "linux_amd64": "beb961aec753e4e9b819e8ab259c30c019229903e27f51b5095322b8c9c0ada7", + "linux_arm64": "1644712fa35b076b7d4b65e4e250fe9a2c01516561841aa362568f3a26f4f0d2", +} -filegroup( - name = "download_test", - srcs = [ - "///darwin_amd64//tools:please_go", - "///darwin_arm64//tools:please_go", - "///freebsd_amd64//tools:please_go", - "///linux_amd64//tools:please_go", - "///linux_arm64//tools:please_go", - ], - labels = ["manual"], -) +for a, h in hashes.items(): + native = f"{CONFIG.OS}_{CONFIG.ARCH}" == a + remote_file( + name = "please_go" if native else f"please_go_{a}", + binary = True, + hashes = [h], + labels = None if native else ["manual"], # Needed to avoid picking this up for other architectures for coverage + url = f"https://github.com/please-build/go-rules/releases/download/please-go-v{VERSION}/please_go-{VERSION}-{a}", + visibility = ["PUBLIC"] if native else None, + ) From 369c48fdc074a0d2afe2ce33678269ebb04f339f Mon Sep 17 00:00:00 2001 From: Chris Novakovic Date: Tue, 27 Jun 2023 11:01:41 +0100 Subject: [PATCH 04/27] Tag v1.7.3 (#139) --- ChangeLog | 6 ++++++ VERSION | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 33f3cf4a..90f8cc12 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +Version 1.7.3 +------------- + * Refactor `please_go` download test to avoid subrepo references (#136) + This fixes fatal errors with `plz query graph` in repos that use go-rules, a regression + present since version 1.7.0. + Version 1.7.2 ------------ * Use correct version of the go tool (#128) diff --git a/VERSION b/VERSION index f8a696c8..661e7aea 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.2 +1.7.3 From 5391cb3659b3650d745b5263f13ace4092cd9656 Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Mon, 3 Jul 2023 15:44:06 +0100 Subject: [PATCH 05/27] Fix issue with coverage redesign + TestMain (#140) --- tools/please_go/ChangeLog | 4 ++++ tools/please_go/VERSION | 2 +- tools/please_go/test/write_test_main.go | 4 +++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tools/please_go/ChangeLog b/tools/please_go/ChangeLog index 8620a5f9..adcb25cd 100644 --- a/tools/please_go/ChangeLog +++ b/tools/please_go/ChangeLog @@ -1,3 +1,7 @@ +Version 1.4.2 +------------- + * Don't run the tests if the user has defined a TestMain in 1.20+ + Version 1.4.1 ------------- * Fix issue where we'd compile the parent directory if the package can't be found diff --git a/tools/please_go/VERSION b/tools/please_go/VERSION index 13175fdc..c9929e36 100644 --- a/tools/please_go/VERSION +++ b/tools/please_go/VERSION @@ -1 +1 @@ -1.4.1 \ No newline at end of file +1.4.2 \ No newline at end of file diff --git a/tools/please_go/test/write_test_main.go b/tools/please_go/test/write_test_main.go index 5e1e8960..45944066 100644 --- a/tools/please_go/test/write_test_main.go +++ b/tools/please_go/test/write_test_main.go @@ -268,8 +268,10 @@ func internalMain() int { {{end}} {{if .Main}} {{.Package}}.{{.Main}}(m) -{{end}} + return 0 +{{else}} return m.Run() +{{end}} } func main() { From c444622c3442b8a71f8120d19b7508a0efb69977 Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Mon, 3 Jul 2023 16:02:33 +0100 Subject: [PATCH 06/27] Set the GOROOT when using go_stdlib() as we use it to find pkg/include (#138) * Set the GOROOT when using go_stdlib() as we use it to find pkg/include * Update version --- build_defs/go.build_defs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index ed94afdd..65718f40 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -1472,18 +1472,15 @@ def _go_pkg_info(name:str, srcs:list, importconfig:str=None, import_path:str="", def _set_go_env(): - if CONFIG.GO.STDLIB: - cmd = 'export GOPATH=$TMP_DIR' + if CONFIG.HOSTOS == 'freebsd': + # FreeBSD has some very strange semantics around hardlinks that lead to it finding + # the wrong thing (essentially os.Executable, which go uses to define GOROOT, returns + # the most recent hardlink to the file). We can work around this way although it's + # not very nice (we don't do this globally because OSX doesn't have realpath). + go_root = f'$(dirname $(dirname $(realpath $TOOLS_GO)))' else: - if CONFIG.HOSTOS == 'freebsd': - # FreeBSD has some very strange semantics around hardlinks that lead to it finding - # the wrong thing (essentially os.Executable, which go uses to define GOROOT, returns - # the most recent hardlink to the file). We can work around this way although it's - # not very nice (we don't do this globally because OSX doesn't have realpath). - go_root = f'$(dirname $(dirname $(realpath $TOOLS_GO)))' - else: - go_root = '$("$TOOLS_GO" env GOROOT)' - cmd = f'export GOPATH=$TMP_DIR && export GOROOT={go_root}' + go_root = '$("$TOOLS_GO" env GOROOT)' + cmd = f'export GOPATH=$TMP_DIR && export GOROOT={go_root}' if CONFIG.GO.C_FLAGS: flags = " ".join(CONFIG.GO.C_FLAGS) From 1234f9512d98f2ca982592f18bf83d245001e29b Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Mon, 3 Jul 2023 16:09:22 +0100 Subject: [PATCH 07/27] Tag v1.7.4 (#141) * Tag v1.7.4 * Update tool * Update release notes --- ChangeLog | 10 ++++++++-- VERSION | 2 +- tools/BUILD | 12 ++++++------ 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/ChangeLog b/ChangeLog index 90f8cc12..bf51ffdc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,8 +1,14 @@ +Version 1.7.4 +------------- + * Fix issues with with tests using TestMain in go1.20+ where we would run the tests + for them. This is the responsibility of TestMain. + * Set GOROOT when compiling stdlib as we need this for setting `pkg/include` for cgo/asm (#138) + Version 1.7.3 ------------- * Refactor `please_go` download test to avoid subrepo references (#136) - This fixes fatal errors with `plz query graph` in repos that use go-rules, a regression - present since version 1.7.0. + This fixes fatal errors with `plz query graph` in repos that use go-rules, a regression + present since version 1.7.0. Version 1.7.2 ------------ diff --git a/VERSION b/VERSION index 661e7aea..10c08801 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.3 +1.7.4 diff --git a/tools/BUILD b/tools/BUILD index e7474792..3b74707d 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -1,11 +1,11 @@ -VERSION = "1.4.1" +VERSION = "1.4.2" hashes = { - "darwin_amd64": "39ba7a7478c16306c7758e81aa53d7ff2908894a8fbc31b1809c2ede7950911e", - "darwin_arm64": "342a23306fca55ec7c3c56ccc96af8e61131897c5ba9c245d2d6c4bc293ee1fd", - "freebsd_amd64": "5273e338aa07f2f2b1a68a08a14acebee52f8a9758f209b0c9deb7cc0a3b90f4", - "linux_amd64": "beb961aec753e4e9b819e8ab259c30c019229903e27f51b5095322b8c9c0ada7", - "linux_arm64": "1644712fa35b076b7d4b65e4e250fe9a2c01516561841aa362568f3a26f4f0d2", + "darwin_amd64": "0df80f71e6a8e8c289c601bdb1937e39960afebbfaf2db5f2dfe32bd3da6beff", + "darwin_arm64": "96722929660b865022caf2cf963ea57d98321bf1d07a9ea1cb4daf901b45fedc", + "freebsd_amd64": "fbd7305a108b2c1abf2eee13189cd4aca23fb97578c909fa7d8614dbc974d6b4", + "linux_amd64": "2df16b7a15c37ea0bffcb82c85ea49c0336138d63d88c67722966b3646fbd5b3", + "linux_arm64": "4afd983b8d9ac848496c5d9ca18c5774435eeeef743155fe0804b3b2673ded96", } for a, h in hashes.items(): From 336cf01756a33d67aef78138714b7360a9ebf7fc Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Thu, 6 Jul 2023 17:39:56 +0100 Subject: [PATCH 08/27] Use tee to avoid issues with flushing when test calls os.Exit() (#142) * Use tee to avoid issues with flushing when test calls os.Exit() * Fix test main * Fix formatting in ChangeLog --- build_defs/go.build_defs | 6 ++---- tools/please_go/ChangeLog | 4 ++++ tools/please_go/VERSION | 2 +- tools/please_go/test/write_test_main.go | 27 +------------------------ 4 files changed, 8 insertions(+), 31 deletions(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index 65718f40..95c60c7a 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -644,7 +644,6 @@ def go_test(name:str, srcs:list, resources:list=None, data:list|dict=None, deps: resources (list): Files to embed in the library using //go:embed directives. data (list|dict): Runtime data files for the test. deps (list): Dependencies - worker (str): Reference to worker script, A persistent worker process that is used to set up the test. visibility (list): Visibility specification flags (str): Flags to apply to the test invocation. sandbox (bool): Sandbox the test on Linux to restrict access to namespaces such as network. @@ -742,9 +741,8 @@ def go_test(name:str, srcs:list, resources:list=None, data:list|dict=None, deps: ) cmds, tools = _go_binary_cmds(static=static, definitions=definitions, gcov=cgo) - test_cmd = f'$TEST {flags}' - if not CONFIG.GO.COVERAGEREDESIGN: - test_cmd += ' 2>&1 | tee $TMP_DIR/test.results' + + test_cmd = '$TEST 2>&1 | tee $TMP_DIR/test.results' worker_cmd = f'$(worker {worker})' if worker else "" if worker_cmd: test_cmd = f'{worker_cmd} && {test_cmd} ' diff --git a/tools/please_go/ChangeLog b/tools/please_go/ChangeLog index adcb25cd..8de52759 100644 --- a/tools/please_go/ChangeLog +++ b/tools/please_go/ChangeLog @@ -1,3 +1,7 @@ +Version 1.4.3 +------------- + * Don't try and pipe test output as it doesn't flush when the TestMain calls os.Exit + Version 1.4.2 ------------- * Don't run the tests if the user has defined a TestMain in 1.20+ diff --git a/tools/please_go/VERSION b/tools/please_go/VERSION index c9929e36..3c80e4f0 100644 --- a/tools/please_go/VERSION +++ b/tools/please_go/VERSION @@ -1 +1 @@ -1.4.2 \ No newline at end of file +1.4.3 \ No newline at end of file diff --git a/tools/please_go/test/write_test_main.go b/tools/please_go/test/write_test_main.go index 45944066..f19450a4 100644 --- a/tools/please_go/test/write_test_main.go +++ b/tools/please_go/test/write_test_main.go @@ -152,7 +152,6 @@ var testMainTmpl = template.Must(template.New("main").Parse(` package main import ( - _gostdlib_io "io" _gostdlib_os "os" {{if not .Benchmark}}_gostdlib_strings "strings"{{end}} _gostdlib_testing "testing" @@ -221,31 +220,7 @@ func coverTearDown(coverprofile string, gocoverdir string) (string, error) { var testDeps = _gostdlib_testdeps.TestDeps{} func internalMain() int { - if resultsFile := _gostdlib_os.Getenv("RESULTS_FILE"); resultsFile != "" { - f, err := _gostdlib_os.Create(resultsFile) - if err != nil { - panic(err) - } - old := _gostdlib_os.Stdout - mw := _gostdlib_io.MultiWriter(f, old) - r, w, err := _gostdlib_os.Pipe() - if err != nil { - panic(err) - } - _gostdlib_os.Stdout = w - done := make(chan struct{}) - go func() { - _gostdlib_io.Copy(mw, r) - done <- struct{}{} - }() - defer func() { - w.Close() - <-done - r.Close() - f.Close() - _gostdlib_os.Stdout = old - }() - } + {{if .Coverage}} coverfile := _gostdlib_os.Getenv("COVERAGE_FILE") args := []string{_gostdlib_os.Args[0], "-test.v", "-test.coverprofile", coverfile} From f3ce5ff0a528774ed47871d1e59b7104d6a9a76c Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Thu, 6 Jul 2023 18:06:08 +0100 Subject: [PATCH 09/27] Tag v1.7.5 (#144) --- ChangeLog | 6 ++++++ VERSION | 2 +- tools/BUILD | 12 ++++++------ 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index bf51ffdc..2f8f2d68 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +Version 1.7.5 +------------- + * Fix issue with missing test results when tests call `os.Exit()` when using go 1.20+. This only + caused the report to miss rows, as the test result file wasn't being flushed, but would never + cause the test to pass when it should have failed. (#142) + Version 1.7.4 ------------- * Fix issues with with tests using TestMain in go1.20+ where we would run the tests diff --git a/VERSION b/VERSION index 10c08801..6a126f40 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.4 +1.7.5 diff --git a/tools/BUILD b/tools/BUILD index 3b74707d..8721e1d6 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -1,11 +1,11 @@ -VERSION = "1.4.2" +VERSION = "1.4.3" hashes = { - "darwin_amd64": "0df80f71e6a8e8c289c601bdb1937e39960afebbfaf2db5f2dfe32bd3da6beff", - "darwin_arm64": "96722929660b865022caf2cf963ea57d98321bf1d07a9ea1cb4daf901b45fedc", - "freebsd_amd64": "fbd7305a108b2c1abf2eee13189cd4aca23fb97578c909fa7d8614dbc974d6b4", - "linux_amd64": "2df16b7a15c37ea0bffcb82c85ea49c0336138d63d88c67722966b3646fbd5b3", - "linux_arm64": "4afd983b8d9ac848496c5d9ca18c5774435eeeef743155fe0804b3b2673ded96", + "darwin_amd64": "9209ecdcf63d8c1b55a19c751fb5db3c6e3886088776b4c58b9ec0e3bc253154", + "darwin_arm64": "ff88728286bd528ce6f798e8ec81bd668248365de8fe73955018498e9a0ba2e9", + "freebsd_amd64": "cb5a9bad2405c6fae2fb64a8a5bc5aee2567b43c79108c77b37558df25cc4950", + "linux_amd64": "d4e670a9803ceba54f8e899d3d3fddb5d41cc3106bec047b4fb8e85e87fd593d", + "linux_arm64": "57c89ebe4da7dfb3401e2be0be7e4a21ffc3d9d866cc198a4479f7c7f734be07", } for a, h in hashes.items(): From abeaeedb19e220f621438f8332af5534976c4203 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Fri, 7 Jul 2023 09:50:34 +0100 Subject: [PATCH 10/27] Re-add flags (#145) --- ChangeLog | 4 ++++ VERSION | 2 +- build_defs/go.build_defs | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2f8f2d68..41dbda76 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +Version 1.7.6 +------------- + * Honour `flags` argument correctly to `go_test` + Version 1.7.5 ------------- * Fix issue with missing test results when tests call `os.Exit()` when using go 1.20+. This only diff --git a/VERSION b/VERSION index 6a126f40..de28578a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.5 +1.7.6 diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index 95c60c7a..10306a86 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -742,7 +742,7 @@ def go_test(name:str, srcs:list, resources:list=None, data:list|dict=None, deps: cmds, tools = _go_binary_cmds(static=static, definitions=definitions, gcov=cgo) - test_cmd = '$TEST 2>&1 | tee $TMP_DIR/test.results' + test_cmd = f'$TEST {flags} 2>&1 | tee $TMP_DIR/test.results' worker_cmd = f'$(worker {worker})' if worker else "" if worker_cmd: test_cmd = f'{worker_cmd} && {test_cmd} ' From 48ab5314ea997e88bc557ae411a8f3185b1becc4 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Jul 2023 11:01:52 +0100 Subject: [PATCH 11/27] optimise string (#147) --- build_defs/go.build_defs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index 10306a86..0258339d 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -672,7 +672,7 @@ def go_test(name:str, srcs:list, resources:list=None, data:list|dict=None, deps: test_package = name + '_lib' # Unfortunately we have to recompile this to build the test together with its library. lib_rule = go_library( - name = '_%s#lib' % name, + name = f'_{name}#lib', srcs = srcs, resources = resources, package = test_package, @@ -729,7 +729,7 @@ def go_test(name:str, srcs:list, resources:list=None, data:list|dict=None, deps: deps += [lib_rule, import_config] lib_rule = go_library( - name='_%s#main_lib' % name, + name=f'_{name}#main_lib', srcs = [main_rule], deps = deps, _needs_transitive_deps = CONFIG.BUILD_CONFIG == "cover" and not CONFIG.GO.COVERAGEREDESIGN, From 863ca1e97f02a914190fd5dd0838b7b03aa9b65e Mon Sep 17 00:00:00 2001 From: Sam Westmoreland Date: Mon, 10 Jul 2023 13:30:42 +0100 Subject: [PATCH 12/27] Run CI steps in parallel and test debug (#134) --- .github/workflows/plugin.yaml | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/.github/workflows/plugin.yaml b/.github/workflows/plugin.yaml index 5b7fc4d3..b7851221 100644 --- a/.github/workflows/plugin.yaml +++ b/.github/workflows/plugin.yaml @@ -8,8 +8,6 @@ jobs: uses: actions/checkout@v2 - name: Run tests run: ./pleasew test -p -v notice --log_file plz-out/log/test.log - - name: Run tests with coverage - run: ./pleasew cover -p -v notice --nocoverage_report --log_file plz-out/log/test.log - name: Archive logs if: always() uses: actions/upload-artifact@v2 @@ -17,6 +15,34 @@ jobs: name: logs path: | plz-out/log + test_coverage: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v2 + - name: Run tests with coverage + run: ./pleasew cover -p -v notice --nocoverage_report --log_file plz-out/log/test_coverage.log + - name: Archive logs + if: always() + uses: actions/upload-artifact@v2 + with: + name: coverage_logs + path: | + plz-out/log + test_debug: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v2 + - name: Build with debug configuration + run: ./pleasew build -c dbg -p -v notice --log_file plz-out/log/test_debug.log + - name: Archive logs + if: always() + uses: actions/upload-artifact@v2 + with: + name: debug_logs + path: | + plz-out/log release: needs: [test] runs-on: ubuntu-latest From ccca57637c3760fa547d85e834bc80a42384758c Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Tue, 11 Jul 2023 14:13:12 +0100 Subject: [PATCH 13/27] Add support for profile-guided optimisation (#148) * Add support for profile-guided optimisation * Version --- .plzconfig | 2 +- ChangeLog | 4 ++++ VERSION | 2 +- build_defs/go.build_defs | 11 ++++++++--- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/.plzconfig b/.plzconfig index 9c052141..ca9cc821 100644 --- a/.plzconfig +++ b/.plzconfig @@ -82,7 +82,7 @@ Help = Changes the test working directory to be the package to be more inline wi [PluginConfig "cpp_coverage"] Type = bool DefaultValue = false -Help = Whether to build C components with coverage +Help = Whether to build C components with coverage [PluginConfig "c_flags"] Inherit = true diff --git a/ChangeLog b/ChangeLog index 41dbda76..4cfa4e00 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +Version 1.8.0 +------------- + * Added `pgo_file` argument to `go_library` which enables profile-guided optimisation. + Version 1.7.6 ------------- * Honour `flags` argument correctly to `go_test` diff --git a/VERSION b/VERSION index de28578a..27f9cd32 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.6 +1.8.0 diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index 0258339d..5d6585bf 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -221,7 +221,7 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: _needs_transitive_deps=False, _all_srcs=False, cover:bool=True, filter_srcs:bool=True, _link_private:bool=False, _link_extra:bool=True, _abi:str=None, _generate_import_config:bool=True, _generate_pkg_info:bool=True, - import_path:str='', labels:list=[], package:str=None): + import_path:str='', labels:list=[], package:str=None, pgo_file:str=None): """Generates a Go library which can be reused by other rules. Args: @@ -244,6 +244,7 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: import_path (str): If set, this will override the import path of the generated go package. package (str): The package as it would appear at the top of the go source files. Defaults to name. + pgo_file (str): The CPU profile to supply for profile-guided optimisation. """ assert srcs, "Cannot provide an empty srcs list to go_library" cover = cover and CONFIG.BUILD_CONFIG == "cover" @@ -430,6 +431,8 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: } else: outs = [out] + if pgo_file: + srcs['pgo'] = [pgo_file] provides = {'go': ':' + name, 'go_src': src_rule} if cover and not CONFIG.GO.COVERAGEREDESIGN: @@ -440,7 +443,7 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: deps = deps, internal_deps = [src_rule], outs = outs, - cmd = _go_library_cmds(import_path=package_path, complete=complete, all_srcs=_all_srcs, cover=cover, filter_srcs=filter_srcs, abi=_abi, embedcfg=embedcfg), + cmd = _go_library_cmds(import_path=package_path, complete=complete, all_srcs=_all_srcs, cover=cover, filter_srcs=filter_srcs, abi=_abi, embedcfg=embedcfg, pgo_file=pgo_file), visibility = visibility, building_description = "Compiling...", requires = ['go'], @@ -1494,7 +1497,7 @@ def _set_go_env(): return cmd -def _go_library_cmds(import_path:str="", complete=True, all_srcs=False, cover=True, filter_srcs=True, abi=False, embedcfg=None): +def _go_library_cmds(import_path:str="", complete=True, all_srcs=False, cover=True, filter_srcs=True, abi=False, embedcfg=None, pgo_file=None): """Returns the commands to run for building a Go library.""" filter_cmd = 'export SRCS_GO="$(\"${TOOLS_PLEASE_GO}\" filter ${SRCS_GO})"; ' if filter_srcs else '' # Invokes the Go compiler. @@ -1505,6 +1508,8 @@ def _go_library_cmds(import_path:str="", complete=True, all_srcs=False, cover=Tr package_flag = f" -p {import_path}" if import_path else "" if CONFIG.GO.RACE: compile_cmd += ' -race' + if pgo_file: + compile_cmd += ' -pgoprofile "$SRCS_PGO"' gen_import_cfg = _set_go_env() if not CONFIG.GO.STDLIB: From ef6a93c36906f7c6a6494be7244eaa66171c5912 Mon Sep 17 00:00:00 2001 From: Sam Westmoreland Date: Tue, 18 Jul 2023 15:12:38 +0100 Subject: [PATCH 14/27] Allow toggling pkg info with a config setting (#150) --- .plzconfig | 5 +++++ ChangeLog | 4 ++++ VERSION | 2 +- build_defs/go.build_defs | 38 +++++++++++++++++++++----------------- 4 files changed, 31 insertions(+), 18 deletions(-) diff --git a/.plzconfig b/.plzconfig index ca9cc821..61625d03 100644 --- a/.plzconfig +++ b/.plzconfig @@ -136,5 +136,10 @@ Help = Compile for the Go race detector Optional = true Help = A built target for a go.mod, which can help avoid the need to pass modules via requirements to go_repo. +[PluginConfig "pkg_info"] +Type = bool +DefaultValue = true +Help = Generate pkg info for Go targets + [Plugin "shell"] Target = //plugins:shell diff --git a/ChangeLog b/ChangeLog index 4cfa4e00..e9600709 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +Version 1.8.1 +------------- + * Add a config option to allow toggling pkg info generation (#150) + Version 1.8.0 ------------- * Added `pgo_file` argument to `go_library` which enables profile-guided optimisation. diff --git a/VERSION b/VERSION index 27f9cd32..a8fdfda1 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.8.0 +1.8.1 diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index 5d6585bf..c9f42c79 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -220,7 +220,7 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: visibility:list=None, test_only:bool&testonly=False, complete:bool=True, _needs_transitive_deps=False, _all_srcs=False, cover:bool=True, filter_srcs:bool=True, _link_private:bool=False, _link_extra:bool=True, _abi:str=None, - _generate_import_config:bool=True, _generate_pkg_info:bool=True, + _generate_import_config:bool=True, _generate_pkg_info:bool=CONFIG.GO.PKG_INFO, import_path:str='', labels:list=[], package:str=None, pgo_file:str=None): """Generates a Go library which can be reused by other rules. @@ -1392,29 +1392,33 @@ def go_module(name:str='', module:str, version:str='', download:str='', deps:lis outs = [f'{name}.importconfig'], ) + provides = { + "go": f":{name}", # provide the filegroup otherwise exported deps don't work + "go_src": download, + "modinfo": download or modinfo, + } + # Package info rule for the Go packages driver - pkg_info = _go_pkg_info( - name = name, - srcs = [download], - importconfig = import_config, - strip = True, - module = module, - test_only = test_only, - install = installpkg, - ) + if CONFIG.GO.PKG_INFO: + pkg_info = _go_pkg_info( + name = name, + srcs = [download], + importconfig = import_config, + strip = True, + module = module, + test_only = test_only, + install = installpkg, + ) + provides["pkg_info"] = pkg_info + exported_deps = exported_deps + [import_config] if exported_deps else [import_config] return filegroup( name = name, srcs = [a_rule], - deps = deps + [pkg_info], + deps = deps + [pkg_info] if CONFIG.GO.PKG_INFO else deps, exported_deps = exported_deps, - provides = { - "go": f":{name}", # provide the filegroup otherwise exported deps don't work - "go_src": download, - "pkg_info": pkg_info, - "modinfo": download or modinfo, - }, + provides = provides, labels = labels + [f"go_package:{i}" for i in install] + ["go_module_path:" + module], visibility = visibility, needs_transitive_deps = True, From afc94bbd15af246fdef3714e8e110ec5062c483c Mon Sep 17 00:00:00 2001 From: Chris Novakovic Date: Thu, 14 Sep 2023 14:57:56 +0100 Subject: [PATCH 15/27] Remove implicit dependency on GCC toolchain (#155) gcov is part of the GCC toolchain - rather than explicitly linking to libgcov in the coverage profile, pass the `--coverage` flag to the compiler toolchain (which is recognised by both GCC and LLVM). --- build_defs/cgo.build_defs | 2 +- build_defs/go.build_defs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/build_defs/cgo.build_defs b/build_defs/cgo.build_defs index f583da15..1676732b 100644 --- a/build_defs/cgo.build_defs +++ b/build_defs/cgo.build_defs @@ -38,7 +38,7 @@ def cgo_library(name:str, srcs:list=[], resources:list=None, go_srcs:list=[], c_ assert srcs or go_srcs, 'At least one of srcs and go_srcs must be provided' if CONFIG.BUILD_CONFIG == "cover": - linker_flags += ["-lgcov"] + linker_flags += ["--coverage"] if not srcs: return go_library( diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index c9f42c79..80d51267 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -1583,7 +1583,7 @@ def _go_binary_cmds(static=False, ldflags='', pkg_config='', definitions=None, g 'opt': f'{gen_import_cfg} && {_link_cmd} {flags} -s -w $SRCS', } if gcov and CONFIG.GO.CPP_COVERAGE: - cmds['cover'] = f'{gen_import_cfg} && {_link_cmd} {flags} -extldflags="-lgcov" $SRCS' + cmds['cover'] = f'{gen_import_cfg} && {_link_cmd} {flags} -extldflags="--coverage" $SRCS' return cmds, { 'go': [CONFIG.GO.GO_TOOL], From e48c780597701c8891fd7e660d8df79529976bc8 Mon Sep 17 00:00:00 2001 From: Chris Novakovic Date: Fri, 15 Sep 2023 09:56:54 +0100 Subject: [PATCH 16/27] Tag v1.8.2 (#156) --- ChangeLog | 4 ++++ VERSION | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index e9600709..6e0e5b60 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +Version 1.8.2 +------------- + * Remove implicit dependency on GCC toolchain when running under coverage profile (#155) + Version 1.8.1 ------------- * Add a config option to allow toggling pkg info generation (#150) diff --git a/VERSION b/VERSION index a8fdfda1..53adb84c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.8.1 +1.8.2 From 8d6e5c331a0a6818dfeeed6295557307c19db5df Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 30 Sep 2023 12:13:48 +0100 Subject: [PATCH 17/27] Update plz version & add subinclude (#159) * Update plz version & add subinclude * Update shell plugin to latest --- .plzconfig | 2 +- plugins/BUILD | 2 +- test/goget/BUILD | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.plzconfig b/.plzconfig index 61625d03..15831477 100644 --- a/.plzconfig +++ b/.plzconfig @@ -1,5 +1,5 @@ [Please] -Version = >=17.0.0-beta.8 +Version = >=17.2.0 [build] hashcheckers = sha256 diff --git a/plugins/BUILD b/plugins/BUILD index 66f99d5d..9251bcb5 100644 --- a/plugins/BUILD +++ b/plugins/BUILD @@ -1,7 +1,7 @@ plugin_repo( name = "e2e", plugin = "plugin-integration-testing", - revision="v1.0.1" + revision="v1.0.2" ) plugin_repo( diff --git a/test/goget/BUILD b/test/goget/BUILD index 4f92ea5e..c2f5f1fb 100644 --- a/test/goget/BUILD +++ b/test/goget/BUILD @@ -1,4 +1,4 @@ -subinclude("//test/build_defs:e2e") +subinclude("///shell//build_defs:shell", "//test/build_defs:e2e") please_repo_e2e_test( name = "go_get_test", From a33a2a593a4f049ad3abe692ada036b21cc62bf6 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 30 Sep 2023 12:14:27 +0100 Subject: [PATCH 18/27] Add modinfo to go_repo actions (#158) * Add modinfo to go_repo * make mod stuff unique --- build_defs/go.build_defs | 28 ++++++++++++++++++++++++---- tools/please_go/generate/generate.go | 9 ++++++++- tools/please_go/generate/rules.go | 2 ++ tools/please_go/modinfo/modinfo.go | 15 ++++++++++----- tools/please_go/please_go.go | 3 ++- 5 files changed, 46 insertions(+), 11 deletions(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index 80d51267..6d1f580d 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -221,7 +221,7 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: _needs_transitive_deps=False, _all_srcs=False, cover:bool=True, filter_srcs:bool=True, _link_private:bool=False, _link_extra:bool=True, _abi:str=None, _generate_import_config:bool=True, _generate_pkg_info:bool=CONFIG.GO.PKG_INFO, - import_path:str='', labels:list=[], package:str=None, pgo_file:str=None): + import_path:str='', labels:list=[], package:str=None, pgo_file:str=None, _module:str=''): """Generates a Go library which can be reused by other rules. Args: @@ -255,6 +255,25 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: private = out.startswith('_') package_path = _get_import_path(package, import_path) + if _module: + # We're in a module, create a modinfo file identifying it + modinfo = build_rule( + name = name, + tag = "modinfo", + outs = [f"_{name}.modinfo"], + cmd = f"echo '{_module}' > $OUT", + ) + else: + # Dummy modinfo so we don't have to provide everything for binary modinfo actions. + modinfo = filegroup( + name = name, + tag = "modinfo", + deps = deps, + requires = ["modinfo"], + test_only = test_only, + ) + deps += [modinfo] + # We could just depend on go_src for all our deps but that would mean bringing in srcs for targets outside this # package like third party sources which is especially slow on systems with slow syscall performance (macOS) if _all_srcs: @@ -1100,8 +1119,9 @@ def _add_ld_flags(name:str, stdout:list): def _module_rule_name(module): return module.replace("/", "_") -def go_repo(module: str, version:str=None, download:str=None, name:str=None, install:list=[], requirements:list=[], - licences=None, patch=None, visibility=["PUBLIC"]): + +def go_repo(module: str, version:str='', download:str=None, name:str=None, install:list=[], requirements:list=[], + licences:list=None, patch:list=None, visibility:list=["PUBLIC"]): """Adds a third party go module to the build graph as a subrepo. This is designed to be closer to how the `go.mod` file works, requiring only the module name and version to be specified. Unlike go_module, each package is compiled individually, and dependencies between packages are inferred by convention. @@ -1166,7 +1186,7 @@ def go_repo(module: str, version:str=None, download:str=None, name:str=None, ins name = name, tag = "repo" if install else None, srcs = srcs, - cmd = f"rm -rf $SRCS_DOWNLOAD/.plzconfig && find $SRCS_DOWNLOAD -name BUILD -delete && $TOOL generate {modFileArg} --module {module} --src_root=$SRCS_DOWNLOAD {install_args} {requirements} && mv $SRCS_DOWNLOAD $OUT", + cmd = f"rm -rf $SRCS_DOWNLOAD/.plzconfig && find $SRCS_DOWNLOAD -name BUILD -delete && $TOOL generate {modFileArg} --module {module} --version '{version}' --src_root=$SRCS_DOWNLOAD {install_args} {requirements} && mv $SRCS_DOWNLOAD $OUT", outs = [subrepo_name], tools = [CONFIG.GO.PLEASE_GO_TOOL], env= { diff --git a/tools/please_go/generate/generate.go b/tools/please_go/generate/generate.go index 77d37689..d0c68eff 100644 --- a/tools/please_go/generate/generate.go +++ b/tools/please_go/generate/generate.go @@ -15,6 +15,7 @@ import ( type Generate struct { moduleName string + moduleArg string srcRoot string buildContext build.Context modFile string @@ -27,7 +28,11 @@ type Generate struct { install []string } -func New(srcRoot, thirdPartyFolder, modFile, module string, buildFileNames, moduleDeps, install []string) *Generate { +func New(srcRoot, thirdPartyFolder, modFile, module, version string, buildFileNames, moduleDeps, install []string) *Generate { + moduleArg := module + if version != "" { + moduleArg += "@" + version + } return &Generate{ srcRoot: srcRoot, buildContext: build.Default, @@ -38,6 +43,7 @@ func New(srcRoot, thirdPartyFolder, modFile, module string, buildFileNames, modu thirdPartyFolder: thirdPartyFolder, install: install, moduleName: module, + moduleArg: moduleArg, } } @@ -341,6 +347,7 @@ func (g *Generate) libRule(pkg *build.Package, dir string) *Rule { name: name, kind: packageKind(pkg), srcs: pkg.GoFiles, + module: g.moduleArg, cgoSrcs: pkg.CgoFiles, compilerFlags: pkg.CgoCFLAGS, linkerFlags: pkg.CgoLDFLAGS, diff --git a/tools/please_go/generate/rules.go b/tools/please_go/generate/rules.go index 057c26df..802af054 100644 --- a/tools/please_go/generate/rules.go +++ b/tools/please_go/generate/rules.go @@ -5,6 +5,7 @@ import "github.com/bazelbuild/buildtools/build" type Rule struct { name string kind string + module string srcs []string cgoSrcs []string compilerFlags []string @@ -54,4 +55,5 @@ func populateRule(r *build.Rule, targetState *Rule) { }, }) } + r.SetAttr("_module", NewStringExpr(targetState.module)) } diff --git a/tools/please_go/modinfo/modinfo.go b/tools/please_go/modinfo/modinfo.go index 18935c1e..1d89dd45 100644 --- a/tools/please_go/modinfo/modinfo.go +++ b/tools/please_go/modinfo/modinfo.go @@ -38,6 +38,7 @@ func WriteModInfo(goTool, modulePath, pkgPath, buildMode, cgoEnabled, goos, goar {Key: "GOOS", Value: goos}, }, } + seen := map[string]struct{}{} if err := filepath.WalkDir(".", func(path string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() || !strings.HasSuffix(path, ".modinfo") { return err @@ -46,11 +47,15 @@ func WriteModInfo(goTool, modulePath, pkgPath, buildMode, cgoEnabled, goos, goar if err != nil { return err } - if module, version, found := strings.Cut(strings.TrimSpace(string(contents)), "@"); found { - bi.Deps = append(bi.Deps, &debug.Module{ - Path: module, - Version: version, - }) + mod := strings.TrimSpace(string(contents)) + if _, present := seen[mod]; !present { + seen[mod] = struct{}{} + if module, version, found := strings.Cut(mod, "@"); found { + bi.Deps = append(bi.Deps, &debug.Module{ + Path: module, + Version: version, + }) + } } return nil }); err != nil { diff --git a/tools/please_go/please_go.go b/tools/please_go/please_go.go index 351260d7..64b54f94 100644 --- a/tools/please_go/please_go.go +++ b/tools/please_go/please_go.go @@ -97,6 +97,7 @@ var opts = struct { ThirdPartyFolder string `short:"t" long:"third_part_folder" description:"The folder containing the third party subrepos" default:"third_party/go"` ModFile string `long:"mod_file" description:"Path to the mod file to use to resolve dependencies against"` Module string `long:"module" description:"The name of the current module"` + Version string `long:"version" description:"The version of the current module"` Install []string `long:"install" description:"The packages to add to the :install alias"` Args struct { Requirements []string `positional-arg-name:"requirements" description:"Any module requirements not included in the go.mod"` @@ -173,7 +174,7 @@ var subCommands = map[string]func() int{ return 0 }, "generate": func() int { - g := generate.New(opts.Generate.SrcRoot, opts.Generate.ThirdPartyFolder, opts.Generate.ModFile, opts.Generate.Module, []string{"BUILD", "BUILD.plz"}, opts.Generate.Args.Requirements, opts.Generate.Install) + g := generate.New(opts.Generate.SrcRoot, opts.Generate.ThirdPartyFolder, opts.Generate.ModFile, opts.Generate.Module, opts.Generate.Version, []string{"BUILD", "BUILD.plz"}, opts.Generate.Args.Requirements, opts.Generate.Install) if err := g.Generate(); err != nil { log.Fatalf("failed to generate go rules: %v", err) } From ebd1e323a6276de49c12671d4c765f9819f1ef37 Mon Sep 17 00:00:00 2001 From: Sylvain Firmery Date: Sat, 30 Sep 2023 13:16:20 +0200 Subject: [PATCH 19/27] fix: dont check toolchain version when version is not provided (#154) as we cannot set a version when url is set --- build_defs/go.build_defs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index 6d1f580d..df2f41e8 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -45,7 +45,7 @@ def go_toolchain(name:str, url:str|dict = '', version:str = '', hashes:list = [] hashes = hashes, ) - if semver_check(version, ">= 1.20.0") and install_std is None: + if version != "" and semver_check(version, ">= 1.20.0") and install_std is None: install_std = True return _go_toolchain( From f9785d354812d60361cf766fb28274a2a7a26352 Mon Sep 17 00:00:00 2001 From: Peter Bocan Date: Sat, 30 Sep 2023 12:38:03 +0100 Subject: [PATCH 20/27] fix: Add warning for missing Go toolchain to README (#151) From Go 1.20 golang no longer ships the precompiled standard library, which means that you must use `go_toolchain` or `go_system_toolchain` --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index ccd31415..01d3a678 100644 --- a/README.md +++ b/README.md @@ -76,3 +76,5 @@ go_binary( Go is especially well suited to writing command line tools and utilities. Binaries can be ran with `plz run`, or used as a tool for other [custom rules](https://please.build/codelabs/genrule/#0). + +**WARNING: From Go 1.20, Golang no longer ships the precompiled standard library and thus you MUST use `go_toolchain` or `go_system_toolchain`.** From 8ce1a90e4e77ba28cdd62c214f7f944d9a6a46ca Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 30 Sep 2023 12:39:16 +0100 Subject: [PATCH 21/27] Make modinfo generation more efficient (#157) * Attempt to make modinfo more efficient * This too * improve * think this should help? * Fix test * Fix some missing modinfo * Fix another one --- build_defs/go.build_defs | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index df2f41e8..c6aa5428 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -262,6 +262,9 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: tag = "modinfo", outs = [f"_{name}.modinfo"], cmd = f"echo '{_module}' > $OUT", + requires = ["modinfo"], + test_only = test_only, + deps = deps, ) else: # Dummy modinfo so we don't have to provide everything for binary modinfo actions. @@ -388,7 +391,7 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: test_only = test_only, labels=labels, ) - provides = {'go': ':' + name, 'go_src': lib_rule} + provides = {'go': ':' + name, 'go_src': lib_rule, 'modinfo': modinfo} if cover and not CONFIG.GO.COVERAGEREDESIGN: provides['cover_vars'] = cover_vars return build_rule( @@ -453,7 +456,7 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs: if pgo_file: srcs['pgo'] = [pgo_file] - provides = {'go': ':' + name, 'go_src': src_rule} + provides = {'go': ':' + name, 'go_src': src_rule, 'modinfo': modinfo} if cover and not CONFIG.GO.COVERAGEREDESIGN: provides['cover_vars'] = cover_vars return build_rule( @@ -1164,7 +1167,7 @@ def go_repo(module: str, version:str='', download:str=None, name:str=None, insta install_args = " ".join([f"--install={i}" for i in install]) if not download: - download = go_mod_download( + download, _ = go_mod_download( name = tag(name, "dl"), module = module, version = version, @@ -1264,6 +1267,9 @@ def go_mod_download(name:str, module:str, version:str, test_only:bool=False, vis tag = "modinfo", outs = [f"_{name}.modinfo"], cmd = f"echo '{module}@{version}' > $OUT", + requires = ["modinfo"], + test_only = test_only, + deps = deps, ) return build_rule( name = name, @@ -1275,6 +1281,9 @@ def go_mod_download(name:str, module:str, version:str, test_only:bool=False, vis tools = { "go": [CONFIG.GO.GO_TOOL], }, + provides = { + "modinfo": modinfo, + }, building_description = 'Fetching...', cmd = ' && '.join(cmds), requires = ['go_src'], @@ -1285,7 +1294,8 @@ def go_mod_download(name:str, module:str, version:str, test_only:bool=False, vis licences = licences, visibility = visibility, deps = deps + [modinfo], - ) + ), modinfo + def go_module(name:str='', module:str, version:str='', download:str='', deps:list=[], exported_deps:list=[], visibility:list=None, test_only:bool=False, binary:bool=False, install:list=[], labels:list=[], @@ -1338,7 +1348,7 @@ def go_module(name:str='', module:str, version:str='', download:str='', deps:lis install = [f"{module}/{i}" if i != "." and i != "" else module for i in install] if not download: - download = go_mod_download( + download, modinfo = go_mod_download( name = name, _tag = "get", module = module, @@ -1351,18 +1361,15 @@ def go_module(name:str='', module:str, version:str='', download:str='', deps:lis strip = strip, patch = patch, ) - outs = [] - - # Modinfo entry for the linker - if not download: - modinfo = build_rule( + else: + modinfo = filegroup( name = name, tag = "modinfo", - outs = [f"_{name}.modinfo"], - cmd = f"echo '{module}@{version}' > $OUT", + deps = deps + [download], + requires = ["modinfo"], test_only = test_only, ) - deps += [modinfo] + outs = [] # Gets the expected archive name given a target package def archive_name(i): @@ -1415,7 +1422,7 @@ def go_module(name:str='', module:str, version:str='', download:str='', deps:lis provides = { "go": f":{name}", # provide the filegroup otherwise exported deps don't work "go_src": download, - "modinfo": download or modinfo, + "modinfo": modinfo, } # Package info rule for the Go packages driver @@ -1463,7 +1470,7 @@ def _go_modinfo(name:str, test_only:bool=False, deps:list): env = {'CGO_ENABLED': '1' if CONFIG.GO.CGO_ENABLED else '0'}, test_only = test_only, deps = deps, - requires = ['modinfo', 'go'], + requires = ['modinfo'], ) From 1adebed2e213cd24183bc0e549712da35e4f3532 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 30 Sep 2023 12:48:45 +0100 Subject: [PATCH 22/27] Tag 1.9.0 (#160) --- ChangeLog | 6 ++++++ VERSION | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 6e0e5b60..8488cc7b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +Version 1.9.0 +------------- + * Fix: don't check toolchain version when a version isn't provided (#154) + * `go_repo` now provides module info so `go version -m` works as expected (#158) + * Go module info build actions are now more efficient with their inputs (#157) + Version 1.8.2 ------------- * Remove implicit dependency on GCC toolchain when running under coverage profile (#155) diff --git a/VERSION b/VERSION index 53adb84c..f8e233b2 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.8.2 +1.9.0 From cad8b260fa5b9f576ed9a7624c14db597be66eac Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 2 Oct 2023 14:04:42 +0100 Subject: [PATCH 23/27] Support :all syntax correctly when embedding (#161) * Add test * Implement --- test/embed/BUILD | 4 ++-- test/embed/embed.go | 8 +++++++- test/embed/embed_test.go | 11 +++++++++++ test/embed/subdir/_test.txt | 1 + tools/please_go/embed/embed.go | 13 ++++++++++++- 5 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 test/embed/subdir/_test.txt diff --git a/test/embed/BUILD b/test/embed/BUILD index af34d8d8..0509edd1 100644 --- a/test/embed/BUILD +++ b/test/embed/BUILD @@ -3,13 +3,13 @@ subinclude("//build_defs:go") go_library( name = "embed", srcs = ["embed.go"], - resources = ["hello.txt"], + resources = ["hello.txt", "subdir"], ) go_test( name = "embed_test", srcs = ["embed_test.go"], - resources = ["hello.txt"], + resources = ["hello.txt", "subdir"], deps = [ ":embed", "//third_party/go:testify", diff --git a/test/embed/embed.go b/test/embed/embed.go index 04dc295e..3a9755b3 100644 --- a/test/embed/embed.go +++ b/test/embed/embed.go @@ -1,6 +1,12 @@ package embed -import _ "embed" +import "embed" //go:embed hello.txt var hello string + +//go:embed subdir +var subdir embed.FS + +//go:embed all:subdir +var subdirAll embed.FS diff --git a/test/embed/embed_test.go b/test/embed/embed_test.go index 48c57eef..7b57dc0a 100644 --- a/test/embed/embed_test.go +++ b/test/embed/embed_test.go @@ -10,3 +10,14 @@ import ( func TestLibEmbed(t *testing.T) { assert.Equal(t, "hello", strings.TrimSpace(hello)) } + +func TestEmbedDir(t *testing.T) { + _, err := subdir.ReadFile("subdir/_test.txt") + assert.Error(t, err) +} + +func TestEmbedDirAll(t *testing.T) { + b, err := subdirAll.ReadFile("subdir/_test.txt") + assert.NoError(t, err) + assert.Equal(t, "hello", strings.TrimSpace(string(b))) +} diff --git a/test/embed/subdir/_test.txt b/test/embed/subdir/_test.txt new file mode 100644 index 00000000..ce013625 --- /dev/null +++ b/test/embed/subdir/_test.txt @@ -0,0 +1 @@ +hello diff --git a/tools/please_go/embed/embed.go b/tools/please_go/embed/embed.go index 5c2a4f8c..679433af 100644 --- a/tools/please_go/embed/embed.go +++ b/tools/please_go/embed/embed.go @@ -7,6 +7,7 @@ import ( "go/build" "io" "io/fs" + "log" "path" "path/filepath" "strings" @@ -54,6 +55,7 @@ func Parse(gofiles []string) (*Cfg, error) { // AddPackage parses a go package and adds any embed patterns to the configuration func (cfg *Cfg) AddPackage(pkg *build.Package) error { for _, pattern := range append(append(pkg.EmbedPatterns, pkg.TestEmbedPatterns...), pkg.XTestEmbedPatterns...) { + log.Printf("here %s", pattern) paths, err := relglob(pkg.Dir, pattern) if err != nil { return err @@ -79,6 +81,13 @@ func dirs(files []string) []string { } func relglob(dir, pattern string) ([]string, error) { + // Go allows prefixing the pattern with all: which picks up files prefixed with . or _ (by default these should be ignored) + includeHidden := false + if strings.HasPrefix(pattern, "all:") { + pattern = strings.TrimPrefix(pattern, "all:") + includeHidden = true + } + paths, err := filepath.Glob(path.Join(dir, pattern)) if err == nil && len(paths) == 0 { return nil, fmt.Errorf("pattern %s: no matching paths found", pattern) @@ -89,7 +98,9 @@ func relglob(dir, pattern string) ([]string, error) { if err != nil { return err } else if !d.IsDir() { - ret = append(ret, strings.TrimLeft(strings.TrimPrefix(path, dir), string(filepath.Separator))) + if hidden := strings.HasPrefix(d.Name(), ".") || strings.HasPrefix(d.Name(), "_"); !hidden || includeHidden { + ret = append(ret, strings.TrimLeft(strings.TrimPrefix(path, dir), string(filepath.Separator))) + } } return nil }); err != nil { From 3ef12a4976576a492a9aae049f2a4b8d07183a61 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 2 Oct 2023 14:30:47 +0100 Subject: [PATCH 24/27] Add direct dependency on modinfo to go_module (#162) * Add dep * update changelog * toolchain update, looks like 1.20.3 might have been deleted * okay go back down to 1.20.8 then --- ChangeLog | 5 +++++ VERSION | 2 +- build_defs/go.build_defs | 2 +- third_party/go/BUILD | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8488cc7b..0c36709b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +Version 1.9.1 +------------- + * Support :all syntax correctly when embedding (#161) + * Minor dependency fix to 1.9.0 + Version 1.9.0 ------------- * Fix: don't check toolchain version when a version isn't provided (#154) diff --git a/VERSION b/VERSION index f8e233b2..9ab8337f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.9.0 +1.9.1 diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index c6aa5428..70c50e71 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -1443,7 +1443,7 @@ def go_module(name:str='', module:str, version:str='', download:str='', deps:lis return filegroup( name = name, srcs = [a_rule], - deps = deps + [pkg_info] if CONFIG.GO.PKG_INFO else deps, + deps = deps + [pkg_info, modinfo] if CONFIG.GO.PKG_INFO else deps, exported_deps = exported_deps, provides = provides, labels = labels + [f"go_package:{i}" for i in install] + ["go_module_path:" + module], diff --git a/third_party/go/BUILD b/third_party/go/BUILD index 115c814c..804d647d 100644 --- a/third_party/go/BUILD +++ b/third_party/go/BUILD @@ -2,7 +2,7 @@ subinclude("//build_defs:go") go_toolchain( name = "toolchain", - version = "1.20.3", + version = "1.20.8", install_std = False, ) From c2d8f3e4666fa2114fa644d34368c936178e4edf Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 2 Oct 2023 16:17:38 +0100 Subject: [PATCH 25/27] Go 1.21 (#163) * update to 1.21 * Emit outputs in the way Go expects * okay just parse the goddamn file to work out the package name i don't know why it can't do this for me since it's parsing the file anyway * we love this test --- third_party/go/BUILD | 8 +++--- tools/please_go/cover/BUILD | 1 + tools/please_go/cover/cover.go | 26 ++++++++++++++++++- .../install/toolchain/toolchain_test.go | 2 +- tools/please_go/please_go.go | 4 +-- 5 files changed, 33 insertions(+), 8 deletions(-) diff --git a/third_party/go/BUILD b/third_party/go/BUILD index 804d647d..04b2c9fd 100644 --- a/third_party/go/BUILD +++ b/third_party/go/BUILD @@ -2,8 +2,8 @@ subinclude("//build_defs:go") go_toolchain( name = "toolchain", - version = "1.20.8", install_std = False, + version = "1.21.1", ) go_stdlib( @@ -184,24 +184,24 @@ go_module( licences = ["BSD-3-Clause"], module = "golang.org/x/term", version = "v0.0.0-20210615171337-6886f2dfbf5b", - deps = [":xsys"], visibility = ["PUBLIC"], + deps = [":xsys"], ) # This is not really necessary, it's here to test the case of a separate go_mod_download # for the gopackagesdriver. go_mod_download( name = "xsync_download", + licences = ["BSD-3-Clause"], module = "golang.org/x/sync", version = "v0.1.0", - licences = ["BSD-3-Clause"], ) go_module( name = "xsync", + download = ":xsync_download", install = ["errgroup"], module = "golang.org/x/sync", - download = ":xsync_download", visibility = ["PUBLIC"], ) diff --git a/tools/please_go/cover/BUILD b/tools/please_go/cover/BUILD index 5413df40..5d4333d7 100644 --- a/tools/please_go/cover/BUILD +++ b/tools/please_go/cover/BUILD @@ -10,4 +10,5 @@ go_library( name = "cover", srcs = ["cover.go"], visibility = ["//tools/please_go/..."], + deps = ["//tools/please_go/install/toolchain"], ) diff --git a/tools/please_go/cover/cover.go b/tools/please_go/cover/cover.go index f06d36a6..bf23577f 100644 --- a/tools/please_go/cover/cover.go +++ b/tools/please_go/cover/cover.go @@ -5,13 +5,22 @@ package cover import ( "bytes" "encoding/json" + "go/parser" + "go/token" "os" "os/exec" + "path/filepath" "strings" + + "github.com/please-build/go-rules/tools/please_go/install/toolchain" ) // WriteCoverage writes the necessary Go coverage information for a set of sources. -func WriteCoverage(goTool, covercfg, output, pkg, pkgName string, srcs []string) error { +func WriteCoverage(goTool, covercfg, output, pkg string, srcs []string) error { + pkgName, err := packageName(srcs[0]) + if err != nil { + return err + } const pkgConfigFile = "pkgcfg" b, _ := json.Marshal(coverConfig{ OutConfig: covercfg, @@ -23,6 +32,11 @@ func WriteCoverage(goTool, covercfg, output, pkg, pkgName string, srcs []string) return err } var buf bytes.Buffer + // 1.21 requires a cover vars file to be written into the output file list + tc := toolchain.Toolchain{GoTool: goTool} + if version, err := tc.GoMinorVersion(); err == nil && version >= 21 { + buf.WriteString(filepath.Join(filepath.Dir(srcs[0]), "_covervars.cover.go\n")) + } for _, src := range srcs { buf.WriteString(strings.TrimSuffix(src, ".go") + ".cover.go\n") } @@ -43,3 +57,13 @@ type coverConfig struct { Granularity string ModulePath string } + +// packageName returns the Go package for a file. +func packageName(filename string) (string, error) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, filename, nil, parser.PackageClauseOnly) + if err != nil { + return "", err + } + return f.Name.Name, nil +} diff --git a/tools/please_go/install/toolchain/toolchain_test.go b/tools/please_go/install/toolchain/toolchain_test.go index 8632b9d6..a88904f6 100644 --- a/tools/please_go/install/toolchain/toolchain_test.go +++ b/tools/please_go/install/toolchain/toolchain_test.go @@ -12,5 +12,5 @@ func TestGetVersion(t *testing.T) { ver, err := tc.GoMinorVersion() require.NoError(t, err) - require.Equal(t, 20, ver) + require.GreaterOrEqual(t, ver, 21) } diff --git a/tools/please_go/please_go.go b/tools/please_go/please_go.go index 64b54f94..e9b4c286 100644 --- a/tools/please_go/please_go.go +++ b/tools/please_go/please_go.go @@ -63,7 +63,7 @@ var opts = struct { CoverageCfg string `short:"c" long:"covcfg" required:"true" description:"Output coveragecfg file to feed into go tool compile"` Output string `short:"o" long:"output" required:"true" description:"File that will contain output names of modified files"` Pkg string `long:"pkg" env:"PKG_DIR" description:"Package that we're in within the repo"` - PkgName string `short:"p" long:"pkg_name" description:"Name of the package we're compiling"` + PkgName string `short:"p" long:"pkg_name" hidden:"true" description:"Deprecated, has no effect"` Args struct { Sources []string `positional-arg-name:"sources" required:"true" description:"Source files to generate embed config for"` } `positional-args:"true"` @@ -154,7 +154,7 @@ var subCommands = map[string]func() int{ return 0 }, "cover": func() int { - if err := cover.WriteCoverage(opts.Cover.GoTool, opts.Cover.CoverageCfg, opts.Cover.Output, opts.Cover.Pkg, opts.Cover.PkgName, opts.Cover.Args.Sources); err != nil { + if err := cover.WriteCoverage(opts.Cover.GoTool, opts.Cover.CoverageCfg, opts.Cover.Output, opts.Cover.Pkg, opts.Cover.Args.Sources); err != nil { log.Fatalf("failed to write coverage: %s", err) } return 0 From 7239967813cc5486104a49f81c0d320edaafb41f Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Mon, 9 Oct 2023 15:36:38 +0100 Subject: [PATCH 26/27] Avoid module on binary (#164) * Don't set _module on go_binary rules in go_repo * Version bump * Refactor --- tools/please_go/ChangeLog | 4 ++++ tools/please_go/VERSION | 2 +- tools/please_go/generate/generate.go | 5 +++-- tools/please_go/generate/rules.go | 6 ++++-- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/tools/please_go/ChangeLog b/tools/please_go/ChangeLog index 8de52759..75834162 100644 --- a/tools/please_go/ChangeLog +++ b/tools/please_go/ChangeLog @@ -1,3 +1,7 @@ +Version 1.4.4 +------------- + * Don't set `_module` on go_binary commands in `go_repo` + Version 1.4.3 ------------- * Don't try and pipe test output as it doesn't flush when the TestMain calls os.Exit diff --git a/tools/please_go/VERSION b/tools/please_go/VERSION index 3c80e4f0..e1df5de7 100644 --- a/tools/please_go/VERSION +++ b/tools/please_go/VERSION @@ -1 +1 @@ -1.4.3 \ No newline at end of file +1.4.4 \ No newline at end of file diff --git a/tools/please_go/generate/generate.go b/tools/please_go/generate/generate.go index d0c68eff..5ee42c5a 100644 --- a/tools/please_go/generate/generate.go +++ b/tools/please_go/generate/generate.go @@ -203,7 +203,7 @@ func (g *Generate) generate(dir string) error { return err } - lib := g.libRule(pkg, dir) + lib := g.ruleForPackage(pkg, dir) if lib == nil { return nil } @@ -336,7 +336,7 @@ func (g *Generate) depTargets(imports []string) []string { return deps } -func (g *Generate) libRule(pkg *build.Package, dir string) *Rule { +func (g *Generate) ruleForPackage(pkg *build.Package, dir string) *Rule { if len(pkg.GoFiles) == 0 && len(pkg.CgoFiles) == 0 { return nil } @@ -356,6 +356,7 @@ func (g *Generate) libRule(pkg *build.Package, dir string) *Rule { hdrs: pkg.HFiles, deps: g.depTargets(pkg.Imports), embedPatterns: pkg.EmbedPatterns, + isCMD: pkg.IsCommand(), } } diff --git a/tools/please_go/generate/rules.go b/tools/please_go/generate/rules.go index 802af054..80d089c6 100644 --- a/tools/please_go/generate/rules.go +++ b/tools/please_go/generate/rules.go @@ -16,7 +16,7 @@ type Rule struct { deps []string embedPatterns []string // TODO(jpoole): handle external test - external bool + external, isCMD bool } func populateRule(r *build.Rule, targetState *Rule) { @@ -55,5 +55,7 @@ func populateRule(r *build.Rule, targetState *Rule) { }, }) } - r.SetAttr("_module", NewStringExpr(targetState.module)) + if !targetState.isCMD { + r.SetAttr("_module", NewStringExpr(targetState.module)) + } } From e992afa0170b7ac1631ba9d3a9e378adbc9db92a Mon Sep 17 00:00:00 2001 From: Jonathan Poole Date: Thu, 12 Oct 2023 13:12:04 +0100 Subject: [PATCH 27/27] Tag v1.9.2 (#165) --- ChangeLog | 5 +++++ VERSION | 2 +- tools/BUILD | 12 ++++++------ 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0c36709b..bc44e63b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +Version 1.9.2 +------------- + * Fix issue with setting _module on go_binary rules within go_repo + + Version 1.9.1 ------------- * Support :all syntax correctly when embedding (#161) diff --git a/VERSION b/VERSION index 9ab8337f..8fdcf386 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.9.1 +1.9.2 diff --git a/tools/BUILD b/tools/BUILD index 8721e1d6..9be94348 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -1,11 +1,11 @@ -VERSION = "1.4.3" +VERSION = "1.4.4" hashes = { - "darwin_amd64": "9209ecdcf63d8c1b55a19c751fb5db3c6e3886088776b4c58b9ec0e3bc253154", - "darwin_arm64": "ff88728286bd528ce6f798e8ec81bd668248365de8fe73955018498e9a0ba2e9", - "freebsd_amd64": "cb5a9bad2405c6fae2fb64a8a5bc5aee2567b43c79108c77b37558df25cc4950", - "linux_amd64": "d4e670a9803ceba54f8e899d3d3fddb5d41cc3106bec047b4fb8e85e87fd593d", - "linux_arm64": "57c89ebe4da7dfb3401e2be0be7e4a21ffc3d9d866cc198a4479f7c7f734be07", + "darwin_amd64": "bb36e910341d0d77a90d6468d46db3ae6af6946dd9f3279fe8dec305d3404516", + "darwin_arm64": "0da19be6e67d1e997f0a1a84f2dfc065993b142c5ef7d4a30cacc4c6cb411b73", + "freebsd_amd64": "9f63aa4db858c2818b8d6bc9ab76bd643833185aa4c6be1162fb5090be07852e", + "linux_amd64": "504c4737b48e4c21de546e1f01d21124fc1a73a2b05c69d93503830da00f9c7a", + "linux_arm64": "f319a8803c7b9d08ecbe955fc7e671c3506319145f5d3ddd96fa8671edd91ebb", } for a, h in hashes.items():