Go modules

[graphaelli]

elastic/beats#15868 is getting close, APM Server will need to adapt soon after.

[axw]

Started looking into this. So far I've only had to make one minor code change, in model/profile, to use the import path github.com/cespare/xxhash/v2.

I'm making my way through build-related changes now. A couple of early findings:

• Beats has some "replace" stanzas which we'll need to make sure are kept up-to-date in apm-server's go.mod. Replacements are not transitive, they're only honoured when they're in the module being built.
• the NOTICE update script still assumes that dependencies are vendored. I think it would be ideal if it referenced the module cache instead, rather than requiring them to be vendored.

[axw]

I modified our Makefile to set

ES_BEATS=$(shell go list -m -f '{{.Dir}}' github.com/elastic/beats)

Which causes "make" to flip out, trying to rebuild some Go files from YACC sources, deep inside the beats vendor dir. This is caused by the apm-server (and apm-server.test) target dependency on $(GO_FILES):

GOFILES = $(shell find . -type f -name '*.go' 2>/dev/null)
GOFILES_NOVENDOR = $(shell find . -type f -name '*.go' -not -path "*/vendor/*" 2>/dev/null)
GOFILES_ALL = $(GOFILES) $(shell find $(ES_BEATS) -type f -name '*.go' 2>/dev/null)
  GOPACKAGES_STRESSTESTS=$(shell find . -name '*.go' 2>/dev/null | xargs awk 'FNR>1 {nextfile} /\+build.*stresstest/ {print FILENAME; nextfile}' | xargs dirname | uniq)

...

${BEAT_NAME}: $(GOFILES_ALL) ## @build build the beat application
          go build $(GOBUILD_FLAGS)

I can't think of a good reason to have those dependencies there. go build will figure out which files have changed, and only rebuild if necessary. We should probably change those to .PHONY targets, and remove the GOFILES_* variables.

There's a crosscompile target where this doesn't really hold true, but I expect people only run that target infrequently - so it should be fine to also make that .PHONY despite it being a more expensive operation.

[axw]

I can't think of a good reason to have those dependencies there. go build will figure out which files have changed, and only rebuild if necessary. We should probably change those to .PHONY targets, and remove the GOFILES_* variables.

Hmm, so: one good reason is that embedding the build time in the binary causes it to be relinked, which is time consuming. IMO it's worth making that optional (off by default, enabled in CI) to avoid having to explicitly list source deps.

[axw]

Targets in beats/dev-tools/mage are passing -mod=vendor to the go tool, forcing the use of vendoring. Unless we're happy to keep vendoring Beats (I'd prefer not to), this should be made configurable.

[axw]

I've created a WIP branch at https://github.com/axw/apm-server/tree/go-modules, which totally removes the apm-server/vendor and apm-server/_beats directories, and adds a go.mod file. Some other major changes:

• Makefile is rewritten to stop including libbeat/scripts/Makefile. Most of what we depend on from libbeat build-wise is in Magefiles, which we continue to use. I have removed everything related to system tests, but haven't yet introduced an alternative. We might move everything system-test related to a separate Makefile which is included, or (my preference) move all system test configuration into the tests themselves.

• Some of the .PHONY targets are rewritten to use proper Make rules/dependencies, e.g. copying files into docs/data, generating fields.yml, and generating apm-server.yml.

• It's necessary to hack beats/dev-tools/mage/settings.go to pick up $ES_BEATS, as the _beats dir is removed

• It's necessary to hack beats/dev-tools/mage/* to stop passing -mod=vendor to the go tool, since I'm not vendoring anything.

Main questions:

• Why is beats vendoring module deps? Is it for expediency so the existing tools work, or is there more to it? Should we continue vendoring even if we're using modules?
• Is it reasonable to stop including libbeat/scripts/Makefile, and go our own way for that bit? At least for me, the existing Makefile is complicated and difficult to understand.

[axw]

Diff for beats (go-modules branch):

$ git diff
diff --git a/dev-tools/mage/fields.go b/dev-tools/mage/fields.go
index be6ce0285..880150a7d 100644
--- a/dev-tools/mage/fields.go
+++ b/dev-tools/mage/fields.go
@@ -101,7 +101,7 @@ func generateFieldsYAML(baseDir, output string, moduleDirs ...string) error {
                return err
        }
 
-       globalFieldsCmd := sh.RunCmd("go", "run", "-mod", "vendor",
+       globalFieldsCmd := sh.RunCmd("go", "run", //"-mod", "vendor",
                filepath.Join(beatsDir, globalFieldsCmdPath),
                "-es_beats_path", beatsDir,
                "-beat_path", baseDir,
diff --git a/dev-tools/mage/settings.go b/dev-tools/mage/settings.go
index dfe674326..efdda52b5 100644
--- a/dev-tools/mage/settings.go
+++ b/dev-tools/mage/settings.go
@@ -275,6 +275,10 @@ func findElasticBeatsDir() (string, error) {
                return repo.RootDir, nil
        }
 
+       if path := os.Getenv("ES_BEATS"); path != "" {
+               return path, nil
+       }
+
        const devToolsImportPath = elasticBeatsImportPath + "/dev-tools/mage"
 
        // Search in project vendor directories. Order is relevant

[simitt]

pinging @kvch for visibility

[kvch]

We decided to keep the vendor folder to avoid CI build failures if the modules cannot be downloaded e.g due to network errors.

I am OK with making -mod=vendor configurable. We will just add -mod=vendor to GOFLAGS, so no modules are fetched. You can keep GOFLAGS empty, so Go will download the dependencies every time it is needed.

BTW from Go 1.14, this flag will become unnecessary anyway, because if there is a vendor folder in the repo, Go will automatically use the folder to compile the binary.

I think it would be ideal if it referenced the module cache instead, rather than requiring them to be vendored.

What do you mean? Is there a module.txt file in the module cache somewhere as well?

[axw]

@kvch thanks for the input!

We decided to keep the vendor folder to avoid CI build failures if the modules cannot be downloaded e.g due to network errors.

Fair enough. At least until we have an internal module mirror, APM Server will probably want to do the same.

I am OK with making -mod=vendor configurable. We will just add -mod=vendor to GOFLAGS, so no modules are fetched. You can keep GOFLAGS empty, so Go will download the dependencies every time it is needed.

BTW from Go 1.14, this flag will become unnecessary anyway, because if there is a vendor folder in the repo, Go will automatically use the folder to compile the binary.

Ah nice, I had missed that. Will Beats set the minimum Go version to 1.14, and remove the flag at that point? Go 1.14 is just around the corner, so it probably makes sense to just go straight to that, rather than adding configuration.

I think it would be ideal if it referenced the module cache instead, rather than requiring them to be vendored.

What do you mean? Is there a module.txt file in the module cache somewhere as well?

Perhaps I'm missing something. Rather than parsing modules.txt, can you instead parse the output of go list -m -json all? Example output:

{
        "Path": "sigs.k8s.io/yaml",
        "Version": "v1.1.1-0.20190704183835-4cd0c284b15f",
        "Time": "2019-07-04T18:38:35Z",
        "Indirect": true,
        "Dir": "/home/andrew/go/pkg/mod/sigs.k8s.io/yaml@v1.1.1-0.20190704183835-4cd0c284b15f",
        "GoMod": "/home/andrew/go/pkg/mod/cache/download/sigs.k8s.io/yaml/@v/v1.1.1-0.20190704183835-4cd0c284b15f.mod",
        "GoVersion": "1.12"
}

You can then follow the Dir value for each, and look for the LICENSE files in there. Dir points into either the module cache, the vendor directory if you're using -mod=vendor, or somewhere else on the local filesystem if you're using a "replace" stanza.

[axw]

After playing with that option a bit, we might not want to use modules.txt or go.mod, because of https://github.com/go-modules-by-example/index/blob/master/010_tools/README.md#tools-as-dependencies.

Some of the build tools we use have licenses incompatible with ours, but they're only used in dev or CI, so they don't actually taint the binaries. If you use go mod vendor, then they still get vendored despite not being linked in, and show up in modules.txt. Similarly, they'll show up in go list -m all.

The alternative would be to parse the output of go list -deps -json ./..., which only lists packages (and their corresponding modules) which match the build tags/constraints.

[axw]

@kvch I created a WIP branch following this line of thought: elastic/beats@go-modules...axw:go-modules

If that looks like a reasonable approach, please let me know and I'll send a PR late next week or early the following one.

[kvch]

The code looks good. Do you mind opening a PR agains go-modules?

[axw]

@kvch I was about to do that today, but then it occurred to me that going through all of the modules is probably the right thing to do -- but only when you do have a vendor directory. Otherwise there's a risk that unacceptably-licensed code ends up vendored in the tree, which is something we avoid as a policy, even when it's not linked into any distributed binaries.