please-build / go-rules Goto Github PK

Apparent on latest version of please-servers

go_repo(
    module = "google.golang.org/api",
    version = "v0.126.0",
)

Leads to:

plz build ///third_party/go/google.golang.org_api//internal:internal
Build stopped after 1.96s. 1 target failed:
    ///third_party/go/google.golang.org_api//internal:internal
Error building target ///third_party/go/google.golang.org_api//internal:internal: exit status 1
pkg/linux_amd64/google.golang.org/api/internal/cba.go, line 43, column 2: could not import github.com/google/s2a-go (open : no such file or directory)

Looks like the generated BUILD file is missing the s2a dep.

If we add another Please project to the build (e.g. a github_repo()), if that please project defines go_repo() targets, those will conflict with the targets in the host repo. I'm not sure what the solution is here. Right now I would probably just leave this as an unsupported feature, but it might be nice if Please could provide a mechanism to discover the repo in the host system, or just no-op in this case.

We currently don't handle cgo comments like this very well:

// #cgo CFLAGS:-I${SRCDIR}/build/include

We expland ${SRCDIR} to the src root which is usualy something like third_party/go/_github.com_some_module#dl. This has a number of problems, 1) we're not allowed # in the directory, and 2) this -I directory needs to be available at that location when we build against it.

This would mean that the include flag here would fail to resolve. Instead we want it to resolve to something absolute like:

 compiler_flags = ["-I"$TMP_DIR"/pkg/linux_amd64/github.com/bytecodealliance/wasmtime-go/v9/build/include"],

We will also need to add that directory as a dep to both the cgo, and cc rules where we pass in these args, to make these files available there too. We will need to parse the compiler and linker flags (-I and -L), and generate filegroups for those directories that we can add as deps where relevant.

NB: there's some pricklyness where build.Import() will expand ${SRCDIR} twice if we pass in a relative directory. This looks like a bug, but i think we just want to replace f"-I{SRCDIR}/{SRCDIR]/build/include" with f"-I$TMP_DIR/{SRCDIR}/build/include":

for i, arg := range args {
	if arg, ok = expandSrcDir(arg, di.Dir); !ok {
		return fmt.Errorf("%s: malformed #cgo argument: %s", filename, arg)
	}
	args[i] = arg
}

switch verb {
case "CFLAGS", "CPPFLAGS", "CXXFLAGS", "FFLAGS", "LDFLAGS":
	// Change relative paths to absolute.
	ctxt.makePathsAbsolute(args, di.Dir)
}

di.Dir gets set to the srcdir, and makePathsAbsolute() makes them absolute relative to the srcdir, resulting in the duplicate srcdir. The only way around this is to do some post-import munging to replace the generated path with the one we want...

///third_party/go/github.com_pierrec_lz4_v4//internal/lz4block:_lz4block#asm
Error building target ///third_party/go/github.com_pierrec_lz4_v4//internal/lz4block:_lz4block#asm: exit status 1
pkg/linux_amd64/github.com/pierrec/lz4/v4/internal/lz4block/decode_amd64.s, line 122: expected pseudo-register; found DI
pkg/linux_amd64/github.com/pierrec/lz4/v4/internal/lz4block/decode_amd64.s, line 255: illegal or missing addressing mode for symbol const_minMatch
asm: assembly of pkg/linux_amd64/github.com/pierrec/lz4/v4/internal/lz4block/decode_amd64.s failed

MWE:

go_repo(module="github.com/pierrec/lz4/v4", version="v4.1.17")

go_get, go_module and now go_repo have always had the ability to refer to a particular version by any commit-ish in the Git source repo, for example:

go_module(
    name = "logrus",
    module = "github.com/sirupsen/logrus",
    version = "d40e25cd45ed9c6b2b66e6b97573a0413e4c23bd",  # This is actually v1.9.3
    # ...
)

The underlying go_mod_download target's modinfo is now derived from the value of version, and this value ultimately ends up in the runtime/debug.BuildInfo struct embedded in the binary that Go outputs:

$ plz build //:gobinary
$ grep -aP '^dep\s+github.com/Sirupsen/logrus' $(plz query outputs //:gobinary)
dep     github.com/Sirupsen/logrus      d40e25cd45ed9c6b2b66e6b97573a0413e4c23bd
dep     github.com/Sirupsen/logrus      d40e25cd45ed9c6b2b66e6b97573a0413e4c23bd

The main consumers of this information - vulnerability scanners - unfortunately don't always know how to handle it, because the expectation is that the value of the third field in a dep entry abides by the Go module version number spec - I can't find anything in the runtime/debug documentation that says this has to be the case, but it's certainly the convention set by the standard Go toolchain. With some scanners (e.g. Trivy), failing to follow that convention results in warnings like this:

2023-08-21T17:11:53.151+0100    WARN    version error (d40e25cd45ed9c6b2b66e6b97573a0413e4c23bd): malformed version: d40e25cd45ed9c6b2b66e6b97573a0413e4c23bd

Some commercial scanners even assume that non-conformant values actually represent v0.0.0, and assume that the linked library is therefore affected by every vulnerability ever reported in that library, regardless of whether that's actually the case.

I suspect the cleanest way to deal with this would be to validate the version number in the modinfo target generated by go_mod_download - rather than just echoing <module>@<version> into the outputted modinfo file, a tiny Go tool could run <version> through golang.org/x/mod/module.Check and write the modinfo file itself if no error is returned or exit fatally if one is. We could either enforce that versions are Go module version numbers now and consider this a breaking change in go-rules, or we could gate it behind a feature flag and leave it disabled by default until v2.

Example seen in puku:

plz query print -f tools //trie:_trie_test#modinfo 
go: go
plz: ///go//tools:please_go

Compared with:

plz query print -f tools //trie:trie
go: //third_party/go:toolchain|go
please_go: ///go//tools:please_go

go tool compile supports splitting the actual compiled code from the export info that other packages need to compile against. This is achieved by passing -linkobj (the -o target then contains only the export info).

Theoretically we could take advantage of this to improve incrementality; altering the implementation of a function but not its signature (or just altering non-exported functions) would then not cause rebuilds of anything downstream except the final binaries.

This isn't super easy to achieve though. Adding the flag & extra output obviously isn't hard but we can't provide a specific output of a target. It could be done by splitting into two actions but then we're doubling the compile work which seems basically worse. It's not dissimilar to how our C++ rules work but they get around it because they can just spit out their headers, whereas here both are outputs of the compiler. We may need some kind of extension to Please to support this.

When I try to build github.com/go-llsqlite/crawshaw
I get the following error:

fatal error: ./c/sqlite3.c: No such file or directory
   20 | // #include "./c/sqlite3.c"
      |           ^~~~~~~~~~~~~~~
compilation terminated.

It looks like please_go_tool doesn't correctly adds c/sqlite3.c in c_srcs

...
    c_srcs = [
        "blocking_step.c",
        "wrappers.c",
    ],
...

Attempt 1: on darwin_arm64

$ plz build ///third_party/go/github.com_klauspost_pgzip//:pgzip
Build stopped after 8.29s. 1 target failed:
    ///third_party/go/github.com_klauspost_pgzip//:pgzip
Error building target ///third_party/go/github.com_klauspost_pgzip//:pgzip: exit status 1
pkg/darwin_amd64/github.com/klauspost/pgzip/gunzip.go, line 26, column 2: could not import github.com/klauspost/compress/flate (open : no such file or directory)
$ 

Attempt 2: on darwin_arm64 cross-compiling to linux_amd64

$ plz build ///third_party/go/github.com_klauspost_pgzip//:pgzip -a linux_amd64
Build stopped after 40ms. 1 target failed:
    ///third_party/go/github.com_klauspost_pgzip_linux_amd64//:pgzip
Subrepo third_party/go/github.com_klauspost_pgzip_linux_amd64 is not defined (referenced by command-line targets)
$ 

Attempt 3: on linux_amd64

$ plz build ///third_party/go/github.com_klauspost_pgzip//:pgzip
Build stopped after 23.32s. 1 target failed:
    ///third_party/go/github.com_klauspost_pgzip//:pgzip
Error building target ///third_party/go/github.com_klauspost_pgzip//:pgzip: exit status 1
pkg/linux_amd64/github.com/klauspost/pgzip/gunzip.go, line 26, column 2: could not import github.com/klauspost/compress/flate (open : no such file or directory)
$ 

The standard go command builds it fine but there appears to be no way of building it using Please 17.3.0.

plz init

mkdir plugins
cat > plugins/BUILD << EOF
plugin_repo(
    name = "go",
    revision="v1.8.1",
)
EOF

cat >> .plzconfig << EOF
[Plugin "go"]
Target = //plugins:go
GoTool = //third_party/go:toolchain|go
EOF

mkdir -p third_party/go
cat > third_party/go/BUILD << EOF
subinclude('///go//build_defs:go')
go_toolchain(name='toolchain', version='1.20.7')
go_repo(module="github.com/klauspost/compress", version="v1.16.7")
go_repo(module="github.com/klauspost/pgzip", version="v1.2.6")
EOF

plz build ///third_party/go/github.com_klauspost_pgzip//:pgzip
plz build ///third_party/go/github.com_klauspost_pgzip//:pgzip -a linux_amd64

This appears to be unconditional here.
There's no way to get around it because it's added after cflags so even if I pass -Werror it undoes it again.

Ideally we'd remove this and just have the minimal amount of stuff needed to make it go.

see https://app.circleci.com/pipelines/github/thought-machine/please/5851/workflows/73babfef-66ea-4fd4-834f-fa98e9b6414d/jobs/46191

It seems to be adding -I /home/runner/... which doesn't exist. I get this locally as well so it's not just expanding $HOME on the CircleCI worker, but I don't get this from the bootstrap version. It feels like this has become hardcoded into the binary at build time somehow, but I can't see what the mechanism is where that might happen.

I've said informally in a bunch of places that I want to do a v2 of this plugin. The changes would include:

• Require go_toolchain (or go_system_toolchain)
• Require go_stdlib to be used
• Remove install_std stuff from go_toolchain
• Possibly simplify/restructure go_toolchain if all that gotool really needs is the binary (little unsure here but I'd like to optimise it better for not passing heaps of files around the place)
• Remove go_module in favour of go_repo
• Remove legacy_imports config option
• Remove coverageredesign config option, make it always on
• Move cgo_test (#217)
• Get rid of all the stdlib-specific stuff in the package driver (it's obsolete once go_stdlib is in use and it doesn't fully work properly now since we can't easily find where the compiled artifacts are)
• The minimum supported Go version is at least v1.20 (for coverageredesign if nothing else)
• Remove any dead code (notably please_go/install and the old paths in please_go/gotest, plus anything in the defs)
• Anything else?

$ plz query graph
17:19:51.073 CRITICAL: Target ///darwin_amd64//tools:please_go not found in build graph

This is on linux_amd64.

Is it possible that Please is trying to add this to the parent repo's build graph and failing?

I guess we just missed that one when they split.

This is techically a breaking change (albeit one people would be unlikely to notice) so we should do it at 2.0 (which we should really get to one day).

Go module dependencies are treated by Please as a unit and do not require specifying all the source files or dependencies.

Could this be extended by pointing to the current repository root or a subdirectory and saying "this is a Go module, apply the same logic"?

I'm struggling mightily with getting the mage tool built using the new regime. I'm coming from:

go_get(
    name = "mage",
    binary = True,
    get = [],
    install = ["github.com/magefile/mage"],
    deps = [":mage_lib"],
)

go_get(
    name = "mage_lib",
    get = "github.com/magefile/mage",
    install = [
        "internal",
        "mage",
        "mg",
        "parse",
        "sh",
        "target",
    ],
    revision = "aedfce64c122eef47009b7f80c9771044753215d",  # v1.8.0
)

And trying to upgrade to:

go_module(
    name = "mage_module",
    module = "github.com/magefile/mage",
    version = "v1.15.0",
    install = [
        "github.com/magefile/mage",
    ],
)

go_binary(
    name = "mage",
    srcs = ["src/github.com/magefile/main.go"],
    deps = [":mage_module"],
)

Results in:

$ plz build
Build stopped after 120ms. 1 target failed:
    //third_party/go:_mage#lib_pkg_info
cannot calculate hash for third_party/go/src/github.com/magefile/main.go: file does not exist

Or:

go_binary(
     name = "mage",
     srcs = ["github.com_magefile_mage/main.go"],
     deps = [
         "///third_party/go/github.com_magefile_mage//main",
         #":mage_module",
     ],
 )

Which results in:

✦ ➜ plz build
Build stopped after 90ms. 1 target failed:
    ///third_party/go/github.com_magefile_mage//main:main
subrepo third_party/go/github.com_magefile_mage is not defined in this package yet. It must appear before it is used by //third_party/go:mage
    ///third_party/go/github.com_magefile_mage//main:main
subrepo third_party/go/github.com_magefile_mage is not defined in this package yet. It must appear before it is used by //third_party/go:mage
    ///third_party/go/github.com_magefile_mage//main:main
subrepo third_party/go/github.com_magefile_mage is not defined in this package yet. It must appear before it is used by //third_party/go:mage

or the other commented-out variant:

 ➜ plz build --shell --debug //third_party/go:mage
11:11:01.203   ERROR: //third_party/go:_mage#lib_pkg_info failed:
cannot calculate hash for third_party/go/github.com_magefile_mage/main.go: file does not exist
Build stopped after 30ms. 1 target failed:
    //third_party/go:_mage#lib_pkg_info
cannot calculate hash for third_party/go/github.com_magefile_mage/main.go: file does not exist

This feels like it should be dead simple, but I'm struggling because I also can't easily investigate the build environment because the --shell is failing to build.

We currently don't have any way to indicate this - we basically run go tool compile without specifying a language version. This means there's no way of opting into features like the new for loop variable scoping. This is presumably going to continue to be a thing in future.

I think this is probably just a config field for the plugin, equivalent to the field in the go.mod file.

This seems to be becoming increasingly relied upon by the wider ecosystem. Currently plz binaries don't contain it and some things won't work with it.

It seems to be fairly simple; create a debug.BuildInfo and chuck it on a line in the importcfg prefixed with modinfo.
We don't have to fill everything in, but I suspect the module info will often be needed. I think we can get what we need there with stamp (assuming there is no dramatic downside to requesting to stamp every single go_binary)

    ///third_party/go/github.com_klauspost_compress//internal/cpuinfo:_cpuinfo#abi
Error building target ///third_party/go/github.com_klauspost_compress//internal/cpuinfo:_cpuinfo#abi: exit status 1
pkg/linux_amd64/github.com/klauspost/compress/internal/cpuinfo/cpuinfo_amd64.s, line 8: #include: open /home/jpoole/please-servers/plz-out/bin/third_party/go/toolchain/pkg/include/go_asm.h: no such file or directory

MWE:

go_repo(module="github.com/klauspost/compress", version="v1.16.5")

re: thought-machine/please#2806

https://pkg.go.dev/embed

If a pattern begins with the prefix ‘all:’, then the rule for walking directories is changed to include those files beginning with ‘.’ or ‘_’. For example, ‘all:image’ embeds both ‘image/.tempfile’ and ‘image/dir/.tempfile’.

We don't support this in the please_go tool

I noticed this issue with the tool. It will panic on my mac when no version is supplied. Is this expected?

➜  core git:(refactor) ✗ plz run ///go//tools/please_go -- get github.com/redis/go-redis/v9       
Upgrading Please from version 17.0.0 to 17.8.0
panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
github.com/please-build/go-rules/tools/please_go/goget.(*getter).goGet(0x1400018fed8?, {0x140001ec940, 0x1, 0x6?})
   tools/please_go/goget/go_get.go:112 +0x26c
github.com/please-build/go-rules/tools/please_go/goget.GoGet(...)
   tools/please_go/goget/go_get.go:130
main.glob..func8()
   tools/please_go/please_go.go:200 +0xd0
main.main()
   tools/please_go/please_go.go:244 +0x84
➜  core git:(refactor) ✗ plz run ///go//tools/please_go -- get github.com/redis/go-redis/[email protected]
Upgrading Please from version 17.0.0 to 17.8.0
go_repo(module="github.com/dgryski/go-rendezvous", version="v0.0.0-20200823014737-9f7001d12a5f")
go_repo(module="github.com/redis/go-redis/v9", version="v9.5.1")
go_repo(module="github.com/bsm/ginkgo/v2", version="v2.12.0")
go_repo(module="github.com/bsm/gomega", version="v1.27.10")
go_repo(module="github.com/cespare/xxhash/v2", version="v2.2.0")

If I configure a go.mod and update it with go1.22 go_repo fails with the following:

2024/03/05 14:06:38 failed to generate go rules: failed to read host repo go.mod "go.mod": go.mod:129: unknown directive: toolchain

Possibly we need to update something to understand the new stanza?

If you define delve as a third party dependency using a go_repo instead of go_module then plz debug fails.

I think this is because with go_repo, a go_binary target is generated for the delve binary whereas the go_module rule generates a build_rule which does the compilation and bypasses go_binary. Both go_binary and go_test adds the delve tool to resulting build_rule's debug_tools when building with "dbg" which means that the delve binary now gets built with a dependency on itself.

I'm thinking we could either:

1. Pass a flag to go_binary and go_test which disables the debug dependencies being added. The please_go BUILD file generator could then pass this flag when generating the BUILD files in the go_repo subrepos.

2. Modify the _debug_cmd helper to not return anything for the configured DELVE_TOOL.

3. seems cleanest. It means that you couldn't run plz debug on a third party dependency but this just feels fine.

Reproduction

Setup

Clone my minimal reproduction repo: https://github.com/marcuscaisey/plz-debug-go-bug.

plz debug //foo:foo_test

Expected Behaviour

Delve spins up.

Type 'help' for list of commands.
(dlv)

Actual Behaviour

plz exits with an error.

Building [0/1, 0.0s]:
  CPU use:   0.0%  I/O:   0.0%  Mem use:   0.0%
11:47:27.211 CRITICAL: Attempted to add ///third_party/go/github.com_go-delve_delve//cmd/dlv:dlv as a dependency of itself.

After upgrading go-rules to 1.16.2, I get the following error:

Error building target ///go.opentelemetry.io_otel//propagation:propagation: exit status 1
pkg/linux_arm64/go.opentelemetry.io/otel/propagation/trace_context.go, line 23, column 2: could not import go.opentelemetry.io/otel/trace (open : no such file or directory)

In 1.15.2 it works,
go.opentelemetry.io_otel/propagation/BUILD.plz diff:

<     deps = [
<         "///go.opentelemetry.io_otel_trace//:trace",
<         "//baggage",
<     ],
---
>     deps = ["//baggage"],

The code generator does some weird things resolving these google.golang.org/genproto/ modules because there's been a migration recently. The deps in the subrepos are based off the go.mod file from the module we're generating for, but that might differ from module to module based on which version we ultimately resolve.

If we're using:

go_repo(module="google.golang.org/genproto", version="v0.0.0-20230306155012-7f2fa6fef1f4")

Then you can build ///third_party/go/google.golang.org_genproto//googleapis/rpc/code:code, but in:

go_repo(module = "google.golang.org/genproto", version = "v0.0.0-20230530153820-e85fd2cbaebc")

This has been moved out to it's own module, so you need this to resolve it to this:
https://pkg.go.dev/google.golang.org/genproto/googleapis/rpc

This can be worked around by passing in requirements = ["google.golang.org/genproto/googleapis/rpc"] but perhaps we should think about something more automatic.

I'm working on a wails project, and I haven't been able to get the package building as a repo in please.

Wails has a lot going on, here's my attempt at making a repo:

go_repo(
    name = "wails",
    module = "github.com/wailsapp/wails/v2",
    version="latest",
    install = [
        "cmd/wails",
        "cmd/wails/internal",
        "cmd/wails/internal/template",
        "cmd/wails/internal/template/base",
        "cmd/wails/flags",
    ],
)

my source with inferred wails deps

When I try to build I get this:
CRITICAL: glob(include=[base], exclude=[]) in plz-out/subrepos/third_party/go/github.com_wailsapp_wails_v2/cmd/wails/internal/template/BUILD returned no files. If this is intended, set allow_empty=True on the glob.

Ported from thought-machine/please#2055