dev: add support for dev generate cgo

[rickystewart]

The contents of these files here is basically cargo-culted from the
Makefile. I have verified that I can get a build working via the
following steps:

./dev generate go cgo
./dev go -- build ./pkg/cmd/cockroach-short

We don't exactly mirror the previous contents of the zcgo_flags.go
files that make would create. One difference is that we don't populate
the zcgo_flags files in go-libedit to configure how the C sources
are built and linked, so we get the default behavior, which is fine, but
means the go build-built binary and the Bazel-built binary will be
using a different compiled archive (maybe compiled with different flags/
a different configuration/etc.) for the libedit sources.

With make, we address this by putting a zcgo_flags_extra.go file in
the sources for go-libedit right in vendor. Post-Bazel we have no
reason for vendor and have no plans to keep it in the long-term so
this is not really suitable. I'm punting on this for now -- the default
behavior will probably be "good enough" for most people.

Closes #77170.

Release note: None

[cockroach-teamcity]

This change is 

[mari-crl]

Hum hum. Makes sense to me, and this change

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rickystewart)

[rickystewart]

bors r=mari-crl

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[knz]

@rickystewart can you explain this part:

the go build-built binary and the Bazel-built binary will be
using a different compiled archive (maybe compiled with different flags/
a different configuration/etc.) for the libedit sources.

It looks to me looking at libedit/BUILD.bazel that the bazel build would use the same flag as specified in zcgo_flag_extra.go:

    copts = ["-DGO_LIBEDIT_NO_BUILD"],

and the various clinkopts.

Would a non-vendored build still use the same BUILD.bazel?

[rickystewart]

@knz We distinguish between three different "ways" to build Cockroach here.

1. The bazel build build.
2. The make build.
3. A go build build bootstrapped by dev generate go cgo (the kind enabled by this PR).

You're correct to notice that our BUILD.bazel are configured such that (1) and (2) will build libedit in the same way (via -DGO_LIBEDIT_NO_BUILD, having cgo link in the prebuilt libedit library). (3) is the odd one out and the one I was describing in that comment. Because dev generate cgo doesn't add the zcgo_flags_extra.go file for go-libedit, that library will be built in the default way by cgo and we won't link in the version of libedit that Bazel would build, as for the other c-deps.

[knz]

I wasn't aware that dev generate cgo was generating libedit-related code. Can you point me where that generation occurs?

[rickystewart]

I wasn't aware that dev generate cgo was generating libedit-related code. Can you point me where that generation occurs?

On the contrary, it is NOT generating libedit-related code.

[knz]

ok let me rephrase: where in the execution of dev generate go cgo do we run a generated cockroach binary?

(i.e., where in the entire chain of dev generate do we ever care about how cockroach is linked?)

[rickystewart]

🤔 I'm not sure what that question means. Taking it at face value, no, dev generate go cgo itself does not build or link cockroach.

[knz]

Ok then if it doesn't, then the libedit zcgo flags would not matter. That's what I was checking ;)

Would it be possible to extend the dev generate {go,cgo} logic with a bazel "forbid" rule, that says "refuse to run the generation if the pkg/cli/clisqlshell go package is involved in a generation step"?

[rickystewart]

We can add a disallowed_imports_test to make sure that none of the build prerequisites for dev generate bazel (as of today those are the c-deps: jemalloc, proj, geos, krb5) depend directly or indirectly on the problem packages like pkg/cli/clisqlshell. Is that what you mean?

[knz]

yes, that is implied by what i mean.

Is the generate bazel target the only one that links+runs go code? (I don't think so: I believe some of our docs and go code generators also link in some go packages from the crdb project)

What I meant is that for all generate targets that import code from the crdb repo, we should ban uses of the clisqlshell package. That includes generate bazel but there may be others, I'm not sure.

[rickystewart]

What I meant is that for all generate targets that import code from the crdb repo, we should ban uses of the clisqlshell package. That includes generate bazel but there may be others, I'm not sure.

I agree the dependencies for dev generate should be as minimal as possible. Unfortunately we are already failing to meet this standard. For example generating the file docs/generated/settings/settings{,-for-tenants}.html (part of dev generate docs) requires building the entire cockroach-short binary.

[knz]

hm ok thanks for explaining.
I think it would be worth creating a followup issue for two things:

• explain somewhere the libedit thing, and explain that "it all works" only because we don't really use the clisqlshell package during codegen (and if we ever break that assumption, we'll need to revisit)
• also, generate a small-er binary to generate the settings etc without a cockroach binary.

[rickystewart]

explain somewhere the libedit thing, and explain that "it all works" only because we don't really use the clisqlshell package during codegen

I don't think this is literally true. dev builds all of its code via Bazel (obviously) and Bazel doesn't care about whatever generated code is in the workspace, including these zcgo_flags.go files. Recall the distinction between the 3 types of builds I mentioned above. Bazel will build its own version of clisqlshell and whether clisqlshell is go build-able doesn't really affect anything.

[knz]

thanks

[erikgrinaker]

gopls keeps complaining about libedit, even after running dev gen:

# github.com/knz/go-libedit/unix
/usr/bin/ld: cannot find -ledit
collect2: error: ld returned 1 exit status
: packages.Load error

From what you're saying, we expect this to resolve itself when we get rid of vendor, yeah?

(fwiw, make buildshort fixed it)

[rickystewart]

From what you're saying, we expect this to resolve itself when we get rid of vendor, yeah?

I've never used gopls, but I think that today you can configure your gopls to not use -mod=vendor and this should fix itself already?

[knz]

@erikgrinaker i'll be curious if you can double-check what the behavior is of gopls as a result of this change on non-bsd, non-macos systems (which both have libedit installed system-wide). If you see the issue you reported above reproduce, and if removing -mod=vendor doesn't alleviate, that would be interesting.

If confirmed, then I suspect a workaround would be for you to install a libedit system-wide which the link directive would pick up. If that workaround works, it would also confirm that there's a bit more work to do in this area.

[erikgrinaker]

Sure, I'll give it a try when I get a chance.

[erikgrinaker]

Seems like dev gen cgo now generates incorrect paths too. For example, pkg/server/status/zcgo_flags.go contains:

// #cgo CPPFLAGS: -I/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/c-deps/libjemalloc/include -I/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/c-deps/libkrb5/include
// #cgo LDFLAGS: -L/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/c-deps/libjemalloc/lib -L/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/c-deps/libproj/lib -L/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/c-deps/libkrb5/lib
import "C"

However, these paths do not exist:

$ ls -l /home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/c-deps/libjemalloc/include
ls: cannot access '/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/c-deps/libjemalloc/include': No such file or directory

$ ls /home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin
build  docs  external  gazelle  gazelle.runfiles  gazelle.runfiles_manifest  gazelle-runner.bash  pkg  _solib_k8
# no c-deps ^

A week-old copy in that tree does exist if I replace com_github_cockroachdb_cockroach with cockroach though:

$ ls -l /home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/execroot/cockroach/bazel-out/k8-fastbuild/bin/c-deps/libjemalloc/include
total 4
dr-xr-xr-x 2 erik erik 4096 Apr 15 09:59 jemalloc

[rickystewart]

@erikgrinaker -- Can you try dev build dev and see if that changes the behavior? Some stuff has changed with the c-deps this week, but I should have addressed that. You might need to dev build dev to pick it up though.

[erikgrinaker]

$ dev build dev
ERROR: open /home/erik/go/src/github.com/cockroachdb/cockroach/bin/dev-versions/dev.29: no such file or directory
$ ls -l bin/dev-versions
ls: cannot access 'bin/dev-versions': No such file or directory

[erikgrinaker]

Nvm, it worked when I did ./dev build dev. I have a symlink bin/dev -> ../dev to get it into my PATH, guess that confused things somehow.

[rickystewart]

I'll push a quick change to bump the dev version so no one else has to manually dev build dev.

[erikgrinaker]

Some stuff has changed with the c-deps this week, but I should have addressed that. You might need to dev build dev to pick it up though.

Yeah, can confirm that cgo stuff works after dev gen cgo go with the new dev version. Thanks!