build: support fedora

[jiceatscion]

Changes can be summarized as follows:
1 . Update the scripts in tools/ so the denecessary packages are installed (in the right order).
2 . Bottleneck references the sqlite lib through a single source file so it's easy to use different implementations of that lib.
3 . Replace mattn_sqlite with modenrc_sqlite to remove dependency on a specific version of glibc (which breaks integration tests).

Also did some minor makefile simplifications: no need to "make go-mod-tidy" and "make go_deps.blz" explicitly. It happens as needed upon "make gazelle".

---

This change is 

[matzf]

Reviewed 40 of 40 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jiceatscion)

---

BUILD.bazel line 22 at r1 (raw file):

# This is simplistic but the complete, by-the-everchanging-bazel-book, solution
# is ludicrously complicated. Go there if and when needed.

Could you describe first in the comment what this tries to achieve and a hint on how to use it?

IIUC, this config_setting matches the gotags defined elsewhere, in the bazelrc or on the command line. Initially I had expected it to define values, like the attribute is called, i.e. to set the gotags. I was very confused for a moment...

---

Makefile line 1 at r1 (raw file):

.PHONY: all antlr bazel clean docker-images gazelle go.mod licenses mocks protobuf scion-topo test test-integration write_all_source_files

go.mod is not phony :)

---

Makefile line 63 at r1 (raw file):

gazelle: go_deps.bzl
	@# call gazelle with -args, which appends our arguments to those from the gazelle() rule
	bazel run //:gazelle --verbose_failures --config=quiet -- -args -mode=$(GAZELLE_MODE) $(GAZELLE_DIRS)

Maybe it would be worth getting rid of these args entirely? The GAZELLE_MODE is only used in lint-go-gazelle below. There, it could be replaced by creating a corresponding gazelle_diff rule in the BUILD file (analogous to the gazelle_update_repos rule above). The antlr thing ignores the GAZELLE_MODE, so we can just remove it there.

---

Makefile line 66 at r1 (raw file):

licenses: bazel
	tools/licenses.sh

Should we also remove the bazel build from the tools/licenses.sh?

---

nogo.json line 113 at r1 (raw file):

            "/com_github_uber_jaeger_client_go": "",
	    "/org_modernc_sqlite": "",
	    "/org_modernc_mathutil": ""

Nit: inconsistent indent (tab)

---

private/storage/db/sqlite_modernc.go line 33 at r1 (raw file):

// no database exists a new database is be created. If the schema version of the
// stored database is different from schemaVersion, an error is returned.
func NewSqlite(path string, schema string, schemaVersion int) (*sql.DB, error) {

Instead of replicating this entire file, we could move only the imports for the sqlite implementations into two separate files. So these two new files would be empty, except for the anonymous import and the build tags.

Perhaps we could add the same constant definition to both files to ensure the build fails if the gotags are not set correctly:

const buildtag_guard = "must choose an sqlite implementation to build, by defining exactly one of the gotags 'sqlite_modernc' or 'sqlite_mattn'"

---

private/storage/db/sqlite_mattn.go line 131 at r1 (raw file):

// that we do not care about. Currently we do not care about ErrConstraint.
// If the error is filtered, we return nil.
func FilterError(err error) error {

FilterError and IsSqliteError is no longer needed after #4372.

[jiceatscion]

Reviewable status: 36 of 40 files reviewed, 5 unresolved discussions (waiting on @matzf)

---

Makefile line 1 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

go.mod is not phony :)

That a make quirck that I had long forgotten. Yes go.mod is phony: it needs to be built regardless of the existence of a file by that name. That is in fact what phony does. Without phony, it doesn't get updated because it has no explicit dependency.

---

Makefile line 63 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Maybe it would be worth getting rid of these args entirely? The GAZELLE_MODE is only used in lint-go-gazelle below. There, it could be replaced by creating a corresponding gazelle_diff rule in the BUILD file (analogous to the gazelle_update_repos rule above). The antlr thing ignores the GAZELLE_MODE, so we can just remove it there.

done

---

Makefile line 66 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Should we also remove the bazel build from the tools/licenses.sh?

Yes, done.

---

nogo.json line 113 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: inconsistent indent (tab)

done

---

private/storage/db/sqlite_modernc.go line 33 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Instead of replicating this entire file, we could move only the imports for the sqlite implementations into two separate files. So these two new files would be empty, except for the anonymous import and the build tags.

Perhaps we could add the same constant definition to both files to ensure the build fails if the gotags are not set correctly:

const buildtag_guard = "must choose an sqlite implementation to build, by defining exactly one of the gotags 'sqlite_modernc' or 'sqlite_mattn'"

The two aren't really identical, the differences are slight but hard to represent as meaningful abstractions. Still, I'll see what I can do about the duplication without making outright spaghetti. Stay tuned.

Re. the constant. Good idea, the error will be clearer than "oh you have two functions named NewSqlite". So, done

---

private/storage/db/sqlite_mattn.go line 131 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

FilterError and IsSqliteError is no longer needed after #4372.

Done.

[jiceatscion]

Reviewable status: 36 of 40 files reviewed, 4 unresolved discussions (waiting on @matzf)

---

BUILD.bazel line 22 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Could you describe first in the comment what this tries to achieve and a hint on how to use it?

IIUC, this config_setting matches the gotags defined elsewhere, in the bazelrc or on the command line. Initially I had expected it to define values, like the attribute is called, i.e. to set the gotags. I was very confused for a moment...

Done.

[jiceatscion]

Reviewable status: 30 of 41 files reviewed, 4 unresolved discussions (waiting on @matzf)

---

private/storage/db/sqlite_modernc.go line 33 at r1 (raw file):

Previously, jiceatscion wrote…

The two aren't really identical, the differences are slight but hard to represent as meaningful abstractions. Still, I'll see what I can do about the duplication without making outright spaghetti. Stay tuned.

Re. the constant. Good idea, the error will be clearer than "oh you have two functions named NewSqlite". So, done

done

[matzf]

Reviewed 3 of 4 files at r2, 7 of 8 files at r3, all commit messages.
Reviewable status: 40 of 41 files reviewed, 3 unresolved discussions (waiting on @jiceatscion)

---

BUILD.bazel line 22 at r1 (raw file):

Previously, jiceatscion wrote…

Done.

💯 thanks!

---

Makefile line 1 at r1 (raw file):

Previously, jiceatscion wrote…

That a make quirck that I had long forgotten. Yes go.mod is phony: it needs to be built regardless of the existence of a file by that name. That is in fact what phony does. Without phony, it doesn't get updated because it has no explicit dependency.

Argh, of course, sorry.

---

private/storage/db/sqlite.go line 22 at r3 (raw file):

	"net/url"

	_ "modernc.org/sqlite"

This is not supposed to be here, or is it?

---

private/storage/db/sqlite_modernc.go line 33 at r1 (raw file):

Previously, jiceatscion wrote…

done

Ah sorry, I missed the differences. Looks good 👍

---

private/storage/db/sqlite_mattn.go line 1 at r3 (raw file):

// Copyright 2023 Scion Association

Suggestion:

// Copyright 2023 SCION Association

---

private/storage/db/sqlite_mattn.go line 31 at r3 (raw file):

// connection path for this sqlite implementation. The modifications turn on 
// foreign keys and WAL journal mode for every SQL query.
func AddPragmas(q url.Values) {

Should very likely not be exported, addPragmas, and driverName.

[jiceatscion]

Reviewable status: 35 of 43 files reviewed, 3 unresolved discussions (waiting on @matzf)

---

private/storage/db/sqlite.go line 22 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

This is not supposed to be here, or is it?

Indeed. That was a leftover. Gone.

---

private/storage/db/sqlite_mattn.go line 1 at r3 (raw file):

// Copyright 2023 Scion Association

Done.

---

private/storage/db/sqlite_mattn.go line 31 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Should very likely not be exported, addPragmas, and driverName.

Done.

[matzf]

Reviewed 3 of 7 files at r4, 2 of 4 files at r5, 1 of 1 files at r6, 4 of 4 files at r9, all commit messages.
Reviewable status: 45 of 46 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

a discussion (no related file):
The section on building with go build in the documentation should now explain that a tag must be given to select the sqlite implementation, here:

scion/doc/dev/setup.rst

Line 116 in 2ce7932

go build -o bin ./<service>/cmd/<service>...

---

[jiceatscion]

Reviewable status: 42 of 47 files reviewed, 1 unresolved discussion (waiting on @matzf)

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

The section on building with go build in the documentation should now explain that a tag must be given to select the sqlite implementation, here:

scion/doc/dev/setup.rst

Line 116 in 2ce7932

go build -o bin ./<service>/cmd/<service>...

Done.

---

[matzf]

Reviewed 1 of 2 files at r7, 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiceatscion)

---

doc/dev/setup.rst line 115 at r10 (raw file):

   - `mattn`: A cgo implementation. It is well established but can makes go
     executables dependent on a minimum glibc version.

Also, maybe "but requires CGo <... link to cgo>_ and makes go executables depend ..."

Suggestion:

   - `mattn`: A cgo implementation. It is well established but makes go
     executables dependent on a minimum glibc version.

[matzf]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiceatscion)

[oncilla]

Reviewed 30 of 41 files at r1, 3 of 4 files at r2, 2 of 8 files at r3, 3 of 7 files at r4, 1 of 2 files at r7, 4 of 4 files at r9, 3 of 4 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion and @matzf)

---

private/storage/db/sqlite_modernc.go line 26 at r11 (raw file):

)

const buildtag_guard_either_sqlite_mattn_or_sqlite_modernc = "must choose an sqlite " +

ditto

---

private/storage/db/sqlite_mattn.go line 26 at r11 (raw file):

)

const buildtag_guard_either_sqlite_mattn_or_sqlite_modernc = "must choose an sqlite " +

variable names in go should always be camelCase instead of snake_case.

See https://go.dev/doc/effective_go#mixed-caps

[oncilla]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion and @matzf)

a discussion (no related file):

Previously, jiceatscion wrote…

Done.

I think having to specify a build flag in all cases is a bit unfortunate.
It also goes against the Go credo that every repository should compile and be able to run go test ./... out of the box.

I would prefer if we agree on a default (modernc or mattn) that is used when no build tag is specified, and enable the other via a build tag instead.

---

[jiceatscion]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf and @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

I think having to specify a build flag in all cases is a bit unfortunate.
It also goes against the Go credo that every repository should compile and be able to run go test ./... out of the box.

I would prefer if we agree on a default (modernc or mattn) that is used when no build tag is specified, and enable the other via a build tag instead.

I don't think go test ./ works out of the box already. There are quite a few preparatory steps required. That said, I am not entirely against having a default choice. (If we want the integration tests to work out of the box for the majority of development environment that would have to be modernc.). However I would find it a bit unfortunate that there be no way to make either choice explicit and to assume there will ever be only two choices.

Because go build constraints are so simplistic I do not know of a way to match the absence of a given category of tags. Should there be more than 2 options for something, we would need to express the default as a negation of every other variants. For example, we would need to put the following in sqlite_modernc.go:

//go:build sqlite_modernc || !(sqlite_mattn || sqlite_x || sqlite_y)

We're really missing a key-value oriented syntax; something like
//go:build sqlite=modernc || sqlite=''

... Unles there's regex matching? Does something like this work?:
//go:build sqlite_modernc || !sqlite_.*

I haven't actually tried.

Opinions?

---

---

private/storage/db/sqlite_mattn.go line 26 at r11 (raw file):

Previously, oncilla (Dominik Roos) wrote…

variable names in go should always be camelCase instead of snake_case.

See https://go.dev/doc/effective_go#mixed-caps

This is meant to be easy to read as part of an error message, thereby providing an obvious hint to the user. We'd loose some of that readability by making it CamelCase. I don't see the point of applying the go convention in this case. This variable isn't referenced except by assigning it to "_" to ensure we get an error when it is undefined, so it has no effect on the readability of the rest of the code.

[jiceatscion]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf and @oncilla)

---

private/storage/db/sqlite_modernc.go line 26 at r11 (raw file):

Previously, oncilla (Dominik Roos) wrote…

ditto

ditto

[jiceatscion]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf and @oncilla)

a discussion (no related file):

Previously, jiceatscion wrote…

I don't think go test ./ works out of the box already. There are quite a few preparatory steps required. That said, I am not entirely against having a default choice. (If we want the integration tests to work out of the box for the majority of development environment that would have to be modernc.). However I would find it a bit unfortunate that there be no way to make either choice explicit and to assume there will ever be only two choices.

Because go build constraints are so simplistic I do not know of a way to match the absence of a given category of tags. Should there be more than 2 options for something, we would need to express the default as a negation of every other variants. For example, we would need to put the following in sqlite_modernc.go:

//go:build sqlite_modernc || !(sqlite_mattn || sqlite_x || sqlite_y)

We're really missing a key-value oriented syntax; something like
//go:build sqlite=modernc || sqlite=''

... Unles there's regex matching? Does something like this work?:
//go:build sqlite_modernc || !sqlite_.*

I haven't actually tried.

Opinions?

After a quick check of the spec... there's no such thing as wildcard matching in go tags and no practical way of implementing anything approximating key-val mapping. So that leaves us with enumerating all alternatives in the default file :-(

---

[jiceatscion]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf and @oncilla)

a discussion (no related file):

Previously, jiceatscion wrote…

After a quick check of the spec... there's no such thing as wildcard matching in go tags and no practical way of implementing anything approximating key-val mapping. So that leaves us with enumerating all alternatives in the default file :-(

So, within these limitations, here's the change that would give you a default sqlite impl:

diff --git a/private/storage/db/sqlite\_modernc.go b/private/storage/db/sqlite\_modernc.go  
index d0b3d0d8c..a621ed286 100644  
\--- a/private/storage/db/sqlite\_modernc.go  
+++ b/private/storage/db/sqlite\_modernc.go  
@@ -12,8 +12,14 @@  
// See the License for the specific language governing permissions and  
// limitations under the License.  
   
\-//go:build sqlite\_modernc  
\-// +build sqlite\_modernc  
+//go:build sqlite\_modernc || !sqlite\_mattn  
+  
+// Note that above go:build expression makes modernc the default by matching  
+// the absence of sqlite\_mattn. Should there be more alternatives, update  
+// the expression to match their absence too.  
+// Also note that this default is overriden by a build configuration  
+// in .bazelrc, so this is only useful when building with "go build" and  
+// may not match what bazel build does.  
   
package db

Pay attention to the caveats that I had to warn about in the following comments. Choose your poison?

---

[oncilla]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf)

a discussion (no related file):

Previously, jiceatscion wrote…

So, within these limitations, here's the change that would give you a default sqlite impl:

diff --git a/private/storage/db/sqlite\_modernc.go b/private/storage/db/sqlite\_modernc.go  
index d0b3d0d8c..a621ed286 100644  
\--- a/private/storage/db/sqlite\_modernc.go  
+++ b/private/storage/db/sqlite\_modernc.go  
@@ -12,8 +12,14 @@  
// See the License for the specific language governing permissions and  
// limitations under the License.  
   
\-//go:build sqlite\_modernc  
\-// +build sqlite\_modernc  
+//go:build sqlite\_modernc || !sqlite\_mattn  
+  
+// Note that above go:build expression makes modernc the default by matching  
+// the absence of sqlite\_mattn. Should there be more alternatives, update  
+// the expression to match their absence too.  
+// Also note that this default is overriden by a build configuration  
+// in .bazelrc, so this is only useful when building with "go build" and  
+// may not match what bazel build does.  
   
package db

Pay attention to the caveats that I had to warn about in the following comments. Choose your poison?

I'm pretty sure we will not add more than these two sqlite implementations.
I would be more than happy to switch to modernc, but maybe let's do that after we have gained some confidence that it works.
Thus, I think the latest suggestion works and is good enough.

As for tests not running with go test ./.... That is an oversight.
I opened #4375 to resolve this. Please let me know if you find any other go unit tests that do not work out of the box.

---

[oncilla]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

---

private/storage/db/sqlite_mattn.go line 26 at r11 (raw file):

Previously, jiceatscion wrote…

This is meant to be easy to read as part of an error message, thereby providing an obvious hint to the user. We'd loose some of that readability by making it CamelCase. I don't see the point of applying the go convention in this case. This variable isn't referenced except by assigning it to "_" to ensure we get an error when it is undefined, so it has no effect on the readability of the rest of the code.

Ack. Resolving because I have no strong opinions on this particular case.
Generally, I prefer keeping to the conventions.

[jiceatscion]

Reviewable status: 45 of 47 files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

I'm pretty sure we will not add more than these two sqlite implementations.
I would be more than happy to switch to modernc, but maybe let's do that after we have gained some confidence that it works.
Thus, I think the latest suggestion works and is good enough.

As for tests not running with go test ./.... That is an oversight.
I opened #4375 to resolve this. Please let me know if you find any other go unit tests that do not work out of the box.

Done.

---

[oncilla]

Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

[oncilla]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

[oncilla]

Dismissed @matzf from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)