build: cockroachdb build is broken on master

[knz]

cc @bufdev @dt

I can't build CockroachDB any more since #66913 went in.

github.com/bufbuild/buf/internal/pkg/interrupt
# github.com/bufbuild/buf/internal/pkg/interrupt
vendor/github.com/bufbuild/buf/internal/pkg/interrupt/interrupt.go:40:25: undefined: signals

I'm going to send a PR upstream (the +build directive in there is bogus) but if it doesn't get merged quick, I'll revert #66913.

[knz]

Here's the upstream PR: bufbuild/buf#363

I'll fork the upstream repo inside the cockroachdb org and revendor that, just in case.

[bufdev]

I don’t think the build directive is bogus - it has been working for years, can you explain what the issue is?

[dt]

I think the issue is that a unix platform which is not darwin, eg FreeBSD, doesn’t match either “linux” or “darwin” in the _unix.go tagged file and of course doesn’t match “windows” either in the _windows.go file, so neither end up matching.

[knz]

I've checked that my upstream PR fixes this. Either it's accepted upstream, or we can use a fork. The added value of buf is too good to revert #66913.

[bufdev]

I think that's the issue bufbuild/buf#362 (comment) however this was intentional in each of these cases - we purposefully meant to limit to darwin and linux for each of these build tags. Each of these build tags protected some OS-specific code that we've only checked to work in darwin and linux, and we want to be opt-in, not opt-out in these situations.

I don't think the fix is !windows, but perhaps it is opting into more unix platforms in each of these files, which shouldn't be too difficult, and we can do pretty quickly.

There's four places:

• protoc/const_unix.go - This just gates the path separator for -I that we actually added in support of 69913, heh. I believe we can easily extend this to openbsd, freebsd, dragonfly without issue.
• interrupt/interrupt_unix.go - This says what signals to intercept for program interruption. I am not as familiar with any of openbsd, freebsd, dragonfly, but I assume we can extend these pretty trivially - if you are familiar, let me know if it's as simple as "these are also `syscall.{SIGINT,SIGTERM} in openbsd, freebsd, dragonfly", but I can also easily figure this out in the Golang source (and will do after commenting here).
• netrc/netrc_unix.go - This just says what the name of the netrc file is by default, which is different in windows. This can likely be extended with no issue, but will check
• app/app_unix.go - This is the one that worries me the most. This gates the names of the stdin, stdout, stderr, and /dev/null equivalent files are in the OS, as well as what the OS equivalent of $HOME, and the OS cache, data, and config directories. With expertise in openbsd, freebsd, and/or dragonfly, these should be also easy to extend, and would love your input, and we can also do some research, but this is the one where it could be trickier.

All solvable, and quickly. We just don't want to do an opt-out !windows flag though, but we'll get right into looking at this. Any input you have is valuable as well.

[bufdev]

Just as an example of the opt-in strategy being used elsewhere: https://github.com/golang/go/blob/master/src/os/sys_unix.go#L6

[bufdev]

Although not the cause of the issue here, another thing to note as we're going through this as: you might want to reconsider your overall tool vendoring strategy here https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/import-tools/main.go as we've seen this cause active issues with Golang tool usage (not specifically buf, but all tools). I know this is a borderline-officially-recommended strategy by Google, but it's provably "incorrect" and causes bugs.

Tools have their own individual dependency pins - as a contrived example, staticcheck, gocovmerge, buf could all have their own specifically-needed pin of github.com/spf13/cobra, which is a library that has been in a decent amount of flux, even post-v1.0. There's been a lot of work around code completions in cobra, and bugs have gotten fixed surrounding code completion on a few different minors. Sometimes, your code might rely on being on a specific version of spf13/cobra, as you rely on their (unbeknownst until now) broken implementation to produce the results you expect. I don't know if staticcheck or gocovmerge use spf13/cobra, but this is just illustrative - this could be true for any library, and is why tools/all programs (including Cockroach) have pins to specific versions of dependencies.

With the "import Golang tools with blank imports and a +tools build directive" strategy, you are now saying to build these Golang tools with the specific versions of the dependencies that Cockroach has, and effectively merging all these dependency pins into a new set of dependency pins for each tool, which can lead to unexpected behavior.

We solve this by effectively doing:

tmp=$(mktemp -d)
cd $tmp
GOBIN=/path/to/install/bin go get github.com/you/tool@YOUR_VERSION
cd -
rm -rf $tmp

https://github.com/bufbuild/buf/blob/0908d63a005fe3aee36bd3a06dda53b65af59c14/make/go/dep_golangci_lint.mk#L12

But there are different strategies around this.

[knz]

All the Unix systems supported by Go are compatible with the code already in buf. There's absolutely no variation in these particular features.

[bufdev]

I don't think this can be said without further investigation - for example, is the filename for stdin always named /dev/stdin, including in i.e. js,wasm?

[knz]

They are the same for all Unix systems.

The js and wasm platforms do not support file Io so many other things in buf are broken for them.

Also look the change I'm proposing is making things work that ought to work and didn't work before. You're pulling strawmen using platforms where the buf binary can't even run today. What is the precise conversation we're having?

[bufdev]

I don't think that's the case - fundamentally, saying that expanding the build tags in these cases to be opt-out is removing a check in the code that is preventing builds on platforms that are unsupported - many other Golang programs including the Golang source itself do the exact same opt-in style for OS-specific code. We're not going to change to opt-out build tags and remove this safety.

The issue you're having is that you are building Buf on platforms we haven't supported yet. We're actively stopping what we're doing and going through the issues I laid out above to come up with a solution for you, the solution of which might be that we expect this to build on unix-like, but that it's not offically supported.

[knz]

If that is too inconvenient on your side, we're comfortable forking buf and maintaining our patchset (we're doing this already for a couple of other dependencies, it's not a big trouble)

[bufdev]

We almost have this solved in a way we're comfortable with for now, see bufbuild/buf#364 for the first half. We'll have another PR up in a short bit.

[knz]

the first PR took care of some of the errors indeed.
I still see:

# github.com/bufbuild/buf/internal/pkg/app
internal/pkg/app/app.go:265:31: undefined: DevStdinFilePath
internal/pkg/app/app.go:270:31: undefined: DevStdoutFilePath
internal/pkg/app/app.go:275:31: undefined: DevStderrFilePath
internal/pkg/app/app.go:280:31: undefined: DevNullFilePath

[bufdev]

Yes - as mentioned on that PR, and in the comment above, we are doing that in a separate PR that will be up in a short bit.

[knz]

amazing, thank you so much for your help here

[bufdev]

This is fixed on master via bufbuild/buf#364 and bufbuild/buf#365 - you can depend on master for now, and this will go out on the next release (master is our release branch though, so depending on it should be fine).

[bufdev]

As a follow up though, I would open a different issue and consider the implications of #67216 (comment) but this is very separate from this.

[knz]

Thank you very much for your high-quality and fast response on this.

[bufdev]

No problem - happy to help - let us know if there are any other issues