sql: assert no dependencies on storage or CGo

[jordanlewis]

No longer necessary for sql to depend on either storage or CGo - let's keep it that way!

All commits but last one from dependent PRs.

Closes #30001.

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[jordanlewis]

Yay, this can merge now.

[jordanlewis]

Yay, this can merge now.

[dt]

🎉

[jordanlewis]

bors r+

[jordanlewis]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)

[benesch]

omg 😍

[benesch]

omg 😍

[lopezator]

Related to #30001 tried to use this commit inside:

https://github.com/lopezator/sqlfmt

To avoid C dependencies, and doesn't seem to work at all still. I am receiving some errors on util library:

vendor/github.com/cockroachdb/cockroach/pkg/util/log/clog.go:1115:40: undefined: build.GetInfo vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:116:7: undefined: build.IsRelease vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:230:5: undefined: build.SeemsOfficial vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:245:10: undefined: build.GetInfo vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:493:6: undefined: build.IsRelease

However it works using CGO, so it looks it already has some kind of C dependency. Only because util library maybe?

[lopezator]

Related to #30001 tried to use this commit inside:

https://github.com/lopezator/sqlfmt

To avoid C dependencies, and doesn't seem to work at all still. I am receiving some errors on util library:

vendor/github.com/cockroachdb/cockroach/pkg/util/log/clog.go:1115:40: undefined: build.GetInfo vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:116:7: undefined: build.IsRelease vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:230:5: undefined: build.SeemsOfficial vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:245:10: undefined: build.GetInfo vendor/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:493:6: undefined: build.IsRelease

However it works using CGO, so it looks it already has some kind of C dependency. Only because util library maybe?

[dt]

@lopezator This change broke the dependency from sql to storage and thus any transitive edge to our C++ dependencies that use a make-driven build process, like rocksdb, and banned them from coming back into the dependencies of sql, but that doesn't include plain old (go-built) cgo.

I think @mjibson has been looking at making the parser package cgo-free but it hasn't proved straightforward: anything that pulls in stdlib's tls, like our base config package or the lib/pq library that our errors package uses pick up a cgo dep via the stdlib's crypto/x509. Also, anything that pulls in our build info package, including our log package, will see cgo as well, since there's a little inline C in there to identify which cgo compiler is in use.

We might be able to eliminate that build C import at this point -- now that we're committed to make driven builds, we can likely just have make inject the c++ compiler used instead of compiling a little inline program to figure it out (cc @benesch?).

However we still have cgo deps via various stdlib packages, namely via tls and crypto/x509 and via net. We can cut the tls edge, both from the pgerror to lib/pq path, since the pgerror helper is only used in tests, and then we could move the base.DocsURL helper, but eventually we'll hit stdlib net and whatever other paths I haven't seen. I'm not sure we can practically avoid any stdlib packages in all our transitive deps, so if they use cgo, we might be out of luck? I dunno, @mjibson has been looking at this longer than I have so maybe he has further insight.

[dt]

@lopezator This change broke the dependency from sql to storage and thus any transitive edge to our C++ dependencies that use a make-driven build process, like rocksdb, and banned them from coming back into the dependencies of sql, but that doesn't include plain old (go-built) cgo.

I think @mjibson has been looking at making the parser package cgo-free but it hasn't proved straightforward: anything that pulls in stdlib's tls, like our base config package or the lib/pq library that our errors package uses pick up a cgo dep via the stdlib's crypto/x509. Also, anything that pulls in our build info package, including our log package, will see cgo as well, since there's a little inline C in there to identify which cgo compiler is in use.

We might be able to eliminate that build C import at this point -- now that we're committed to make driven builds, we can likely just have make inject the c++ compiler used instead of compiling a little inline program to figure it out (cc @benesch?).

However we still have cgo deps via various stdlib packages, namely via tls and crypto/x509 and via net. We can cut the tls edge, both from the pgerror to lib/pq path, since the pgerror helper is only used in tests, and then we could move the base.DocsURL helper, but eventually we'll hit stdlib net and whatever other paths I haven't seen. I'm not sure we can practically avoid any stdlib packages in all our transitive deps, so if they use cgo, we might be out of luck? I dunno, @mjibson has been looking at this longer than I have so maybe he has further insight.

[benesch]

We might be able to eliminate that build C import at this point -- now that we're committed to make driven builds, we can likely just have make inject the c++ compiler used instead of compiling a little inline program to figure it out (cc @benesch?).

Definitely an option, but I'm not sure it buys us much. These packages whose dependency sets we're minimizing shouldn't be depending on build anyway. My laptop is currently out of commission while it upgrades to Mojave, but once I get it back I'll do some digging on where the edge to build is coming from.

It's ok to use the net package. Go has some crazy shenanigans where it will bundle its own DNS resolver that doesn't require cgo if you compile net with CGO_ENABLED=0.

tl;dr I think making it so that these library packages can be compiled without cgo is totally within reach.

[benesch]

We might be able to eliminate that build C import at this point -- now that we're committed to make driven builds, we can likely just have make inject the c++ compiler used instead of compiling a little inline program to figure it out (cc @benesch?).

Definitely an option, but I'm not sure it buys us much. These packages whose dependency sets we're minimizing shouldn't be depending on build anyway. My laptop is currently out of commission while it upgrades to Mojave, but once I get it back I'll do some digging on where the edge to build is coming from.

It's ok to use the net package. Go has some crazy shenanigans where it will bundle its own DNS resolver that doesn't require cgo if you compile net with CGO_ENABLED=0.

tl;dr I think making it so that these library packages can be compiled without cgo is totally within reach.

[dt]

the edge to build is pervasive: the most direct edge here is via the base package but everything gets to logging eventually and logging get to build to determine if it is a release/dev build. I guess we could buildtag out the inline cgo in build ourselves for non-cgo builds?

[dt]

the edge to build is pervasive: the most direct edge here is via the base package but everything gets to logging eventually and logging get to build to determine if it is a release/dev build. I guess we could buildtag out the inline cgo in build ourselves for non-cgo builds?

[lopezator]

stdlib (CGO or not, as @benesch said shouldn't be a problem, I think).

The offending parts I detected for my particular usecase of sql package to use in:

https://github.com/lopezator/sqlfmt

Are:

https://github.com/cockroachdb/cockroach/blob/master/pkg/util/tracing/annotate.go#L17-L19

And the one mentioned by @dt :

https://github.com/cockroachdb/cockroach/blob/master/pkg/build/info.go#L26-L35

Removing them, makes my sqlfmt free of any CGO, and thus, capable of building using standard go and without needing any C paraphernalia, i.e:

GOOS=windows GOARCH=amd64 go build cmd/sqlfmt/main.go
GOOS=darwin GOARCH=amd64 go build cmd/sqlfmt/main.go

etc

[lopezator]

stdlib (CGO or not, as @benesch said shouldn't be a problem, I think).

The offending parts I detected for my particular usecase of sql package to use in:

https://github.com/lopezator/sqlfmt

Are:

https://github.com/cockroachdb/cockroach/blob/master/pkg/util/tracing/annotate.go#L17-L19

And the one mentioned by @dt :

https://github.com/cockroachdb/cockroach/blob/master/pkg/build/info.go#L26-L35

Removing them, makes my sqlfmt free of any CGO, and thus, capable of building using standard go and without needing any C paraphernalia, i.e:

GOOS=windows GOARCH=amd64 go build cmd/sqlfmt/main.go
GOOS=darwin GOARCH=amd64 go build cmd/sqlfmt/main.go

etc

[mjibson]

@benesch the build package is annoyingly common. Over 100 packages have transitive dependencies on it including almost everything in sql, storage, util, server. It's because of two reasons: the log package uses it on crashes and a few things in SQL use it to get the docs URL of the current version. I tried mjibson@d1a209d at one point which causes build to poke into other packages to remove the dependency on it, but I'm not convinced it's a good idea, or maybe there's a better way.

[mjibson]

@benesch the build package is annoyingly common. Over 100 packages have transitive dependencies on it including almost everything in sql, storage, util, server. It's because of two reasons: the log package uses it on crashes and a few things in SQL use it to get the docs URL of the current version. I tried mjibson@d1a209d at one point which causes build to poke into other packages to remove the dependency on it, but I'm not convinced it's a good idea, or maybe there's a better way.

[dt]

@mjibson #31467

[dt]

@mjibson #31467