Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate Go plugin with build system. #17005

Merged
merged 18 commits into from Feb 26, 2024
Merged

Conversation

Ferroin
Copy link
Member

@Ferroin Ferroin commented Feb 13, 2024

Summary

This consists of the following parts:

  • A custom CMake Find module to locate the Go toolchain. This allows us to handle checking for the Go toolchain cleanly just using find_package(Go). This includes a special case to look for the go command in /usr/local/go/bin, since that’s where the official toolchain is usually installed if it’s not been installed through the system package manager (and is also where our code will attempt to install it if a usable copy is not found).
  • A second CMake module to provide a macro that simplifies adding Go targets. This encapsulates determining the file dependencies, adding the requisite custom command, and adding a custom target so that the custom command actually gets invoked.
  • A new option to control whether the Go plugin is enabled or not, called ENABLE_PLUGIN_GO. This is enabled by default.
  • Support code in the installer script that will check for and attempt to fetch a usable Go toolchain. If none is found and a copy of the official toolchain can’t be installed, the Go plugin will be automatically disabled.
  • Packaging modifications to make the build work correctly:
    • A new build dependency on a sufficiently recent version of Go. Our package builder images will mask this dependency since we bundle a new enough toolchain in them to not need a system copy.
    • Handling of the artifacts built now as part of the regular build process.
    • Complete removal of the ‘old’ Go handling code.
  • Minor changes to the Coverity scan script so that it skips the Go code (since we don’t use Coverity with that).
  • A few additions to the packaging/check-for-go-toolchain.sh script so that it will allow updating a locally installed copy of the Go toolchain that it installed itself (this allows for seamless handling when we increase the minimum toolchain version).
Test Plan

Preliminary testing consists of verifying that CI passes on this PR.

Secondary testing requires manually building in a variety of different setups.

Additional Information

This is the main remaining part of #16808.

Once merged there are still some places we can improve things, such as caching Go code dependencies between CI runs.

@github-actions github-actions bot added area/packaging Packaging and operating systems support area/tests area/build Build system (autotools and cmake). labels Feb 13, 2024
@Ferroin
Copy link
Member Author

Ferroin commented Feb 14, 2024

Rebased to pick up latest changes in master.

@thiagoftsm

This comment was marked as duplicate.

@Ferroin
Copy link
Member Author

Ferroin commented Feb 14, 2024

Hello @Ferroin ,

Please, take a look in Coverity.

The Coverity failure is because of limitations of the CI infrastructure, not changes in the PR.

On FreeBSD I have this:

The following warnings and non-fatal errors were encountered during the installation process:

  - Go 1.21.0 needed to build Go plugin, but could not find or install a usable toolchain: We do not support automatic handling of a Go toolchain on this system, you must install one manually.

Should we update our install-required-packages.sh to fix this?

We’re very intentionally not handling this in the install-required-packages.sh script, because the minimum required version is a property of the sources being used to build Netdata. On Linux systems it should be pulling in a copy of the official Go toolchain if it can’t find one on the system, we just didn’t get around to adding support for this for FreeBSD or macOS yet.

You should be getting a ‘regular’ install without the Go plugin in this case, and can mask the warning by just adding --disable-go to the install options or installing a new enough Go toolchain.

@thiagoftsm
Copy link
Contributor

thiagoftsm commented Feb 15, 2024

I also have this issue on Slackware:

go: downloading github.com/emicklei/go-restful/v3 v3.11.0
go: downloading github.com/mailru/easyjson v0.7.7
go: downloading github.com/josharian/intern v1.0.0
go: downloading github.com/go-openapi/jsonpointer v0.19.6
/root/go/pkg/mod/github.com/lmittmann/tint@v1.0.4/handler.go:60:2: package log/slog is not in GOROOT (/usr/src/log/slog)

When I try to compile with:

root@hades:/home/thiago/Netdata/tests_netdata# go version
go version go1.18 gccgo (GCC) 13.2.0 linux/amd64

Should we only compile when the go versions is expected?

Best regards!

@Ferroin
Copy link
Member Author

Ferroin commented Feb 15, 2024

I also have this issue on Slackware:

go: downloading github.com/emicklei/go-restful/v3 v3.11.0
go: downloading github.com/mailru/easyjson v0.7.7
go: downloading github.com/josharian/intern v1.0.0
go: downloading github.com/go-openapi/jsonpointer v0.19.6
/root/go/pkg/mod/github.com/lmittmann/tint@v1.0.4/handler.go:60:2: package log/slog is not in GOROOT (/usr/src/log/slog)

I’m not sure what’s up with this.

When I try to compile with:

root@hades:/home/thiago/Netdata/tests_netdata# go version
go version go1.18 gccgo (GCC) 13.2.0 linux/amd64

Should we only compile when the go versions is expected?

We currently don’t support GCCGO, only the upstream toolchain. You should actually be getting an error trying to build in this case because we require Go 1.21 or newer, but it looks like parsing the version string may be failing in this case.

@thiagoftsm
Copy link
Contributor

We currently don’t support GCCGO, only the upstream toolchain. You should actually be getting an error trying to build in this case because we require Go 1.21 or newer, but it looks like parsing the version string may be failing in this case.

I can confirm that using go from go lang project I can overcome this step. I still had another issue, but I stopped to work in another PR. I will bring updates soon.

@ilyam8
Copy link
Member

ilyam8 commented Feb 15, 2024

/root/go/pkg/mod/github.com/lmittmann/tint@v1.0.4/handler.go:60:2: package log/slog is not in GOROOT (/usr/src/log/slog)

log/slog was added to stdlib in 1.21

@thiagoftsm
Copy link
Contributor

Hello @Ferroin ,

After last commits, the compilation on FreeBSD is failing with:

 --- Compile netdata --- 
[/home/thiago/Netdata/netdata]# /usr/local/bin/cmake --build ./build/ --parallel 2 -- VERBOSE=1 
invalid value "'-w -s -X main.version=1.44.0.370'" for flag -ldflags: parameter may not start with quote character '
usage: go build [-o output] [build flags] [packages]
Run 'go help build' for details.
gmake[2]: *** [CMakeFiles/go-plugin.dir/build.make:815: go.d.plugin] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:405: CMakeFiles/go-plugin.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....
gmake: *** [Makefile:136: all] Error 2
 FAILED  ''

Best regards!

@thiagoftsm
Copy link
Contributor

Hello @Ferroin ,

After last commits, when I try to compile with gcc go I have:

[497/501] cd /home/thiago/Netdata/tests_netdata/src/go/collectors/go.d.plugin && /usr/bin/cmake -E env CGO_ENABLED=0 /usr/bin/go build -ldflags "-w -s -X main.version=1.44.0.370" -o /home/thiago/Netdata/tests_netdata/build/go.d.plugin ./cmd/godplugin
FAILED: go.d.plugin /home/thiago/Netdata/tests_netdata/build/go.d.plugin 
cd /home/thiago/Netdata/tests_netdata/src/go/collectors/go.d.plugin && /usr/bin/cmake -E env CGO_ENABLED=0 /usr/bin/go build -ldflags "-w -s -X main.version=1.44.0.370" -o /home/thiago/Netdata/tests_netdata/build/go.d.plugin ./cmd/godplugin
/root/go/pkg/mod/github.com/lmittmann/tint@v1.0.4/handler.go:60:2: package log/slog is not in GOROOT (/usr/src/log/slog)

And the compilation ends with the following error:

ninja: build stopped: subcommand failed.

I am going to test on FreeBSD again. 🤝 .

Best regards!

@thiagoftsm
Copy link
Contributor

The FreeBSD error is different now:

error obtaining VCS status: exit status 128
        Use -buildvcs=false to disable VCS stamping.
gmake[2]: *** [CMakeFiles/go-plugin.dir/build.make:815: go.d.plugin] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:405: CMakeFiles/go-plugin.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

best regards!

@thiagoftsm
Copy link
Contributor

We stilll have the same issues with gcc go:

FAILED: go.d.plugin /home/thiago/Netdata/tests_netdata/build/go.d.plugin 
cd /home/thiago/Netdata/tests_netdata/src/go/collectors/go.d.plugin && /usr/bin/cmake -E env CGO_ENABLED=0 /usr/bin/go build -ldflags "-w -s -X main.version=1.44.0.370" -o /home/thiago/Netdata/tests_netdata/build/go.d.plugin ./cmd/godplugin
/root/go/pkg/mod/github.com/lmittmann/tint@v1.0.4/handler.go:60:2: package log/slog is not in GOROOT (/usr/src/log/slog)
agent/filestatus/store.go:9:2: package slices is not in GOROOT (/usr/src/slices)
[500/501] /usr/bin/cc -D_GNU_SOURCE -I/home/thiago/Netdata/tests_netdata/externaldeps/protobuf/src -I/home/thiago/Netdata/tests_netdata/src/aclk/aclk-schemas -I/usr/include/libmongoc-1.0 -I/usr/include/libbson-1.0 -I/usr/include/uuid -I/usr/include/json-c -I/home/thiago/Netdata/tests_netdata/externaldeps/libbpf/include -I/home/thiago/Netdata/tests_netdata/externaldeps/libbpf/include/uapi -I/home/thiago/Netdata/tests_netdata/build -I/home/thiago/Netdata/tests_netdata -I/home/thiago/Netdata/tests_netdata/src -I/home/thiago/Netdata/tests_netdata/src/libnetdata/libjudy/src -I/home/thiago/Netdata/tests_netdata/src/libnetdata/libjudy/src/JudyCommon -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/include -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/cloexec -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/brotli/enc -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/golombset -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/libgkc -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/libyrmcds -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/klib -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/neverbleed -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/picohttpparser -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/picotest -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/yaml/include -I/home/thiago/Netdata/tests_netdata/src/web/server/h2o/libh2o/deps/yoml -I/home/thiago/Netdata/tests_netdata/aclk/helpers -O1 -ggdb -Wall -Wextra -fno-omit-frame-pointer -Wformat-signedness -fstack-protector-all -Wformat-truncation=2 -Wunused-result -DENABLE_HTTPD=1 -DNETDATA_DEV_MODE=1 -DNETDATA_VERIFY_LOCKS=1 -DMQTT_WSS_DEBUG=1 -DFLB_HAVE_INOTIFY -fexceptions -O3 -DNDEBUG  -D_FORTIFY_SOURCE=3 -fstack-clash-protection -fcf-protection=full  -ffunction-sections -fdata-sections -std=gnu11 -DH2O_USE_LIBUV=0 -DMQTT_WSS_CUSTOM_ALLOC -DRBUF_CUSTOM_MALLOC -DMQTT_WSS_CPUSTATS -MD -MT CMakeFiles/netdata.dir/src/database/sqlite/sqlite3.c.o -MF CMakeFiles/netdata.dir/src/database/sqlite/sqlite3.c.o.d -o CMakeFiles/netdata.dir/src/database/sqlite/sqlite3.c.o -c /home/thiago/Netdata/tests_netdata/src/database/sqlite/sqlite3.c
ninja: build stopped: subcommand failed.

Best regards!

@thiagoftsm
Copy link
Contributor

The FreeBSD error is different now:

error obtaining VCS status: exit status 128
        Use -buildvcs=false to disable VCS stamping.
gmake[2]: *** [CMakeFiles/go-plugin.dir/build.make:815: go.d.plugin] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:405: CMakeFiles/go-plugin.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

best regards!

This is still an issue on FreeBSD.

)
if (RESULT EQUAL 0)
string(REGEX MATCH "go([0-9]+\\.[0-9]+(\\.[0-9]+)?)" GO_VERSION_STRING "${GO_VERSION_STRING}")
string(REGEX MATCH "([0-9]+\\.[0-9]+(\\.[0-9]+)?)" GO_VERSION_STRING "${GO_VERSION_STRING}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need regex that doesn't start with go?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To extract the actual version number.

The string has to be just the version number, nothing else, so that CMake can process it properly. The original revision actually had just the second match, the first one was added to try and properly handle GCCGO which produces a very different version string from the official toolchain.

@Ferroin
Copy link
Member Author

Ferroin commented Feb 16, 2024

The FreeBSD error is different now:

error obtaining VCS status: exit status 128
        Use -buildvcs=false to disable VCS stamping.
gmake[2]: *** [CMakeFiles/go-plugin.dir/build.make:815: go.d.plugin] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:405: CMakeFiles/go-plugin.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

best regards!

This is still an issue on FreeBSD.

@ilyam8 Do we have a particular need for VCS stamping for the binaries?

@Ferroin
Copy link
Member Author

Ferroin commented Feb 16, 2024

@thiagoftsm The GCCGO case should just fail during the configure step now, and the error you reported on FreeBSD should be resolved.

@thiagoftsm
Copy link
Contributor

@thiagoftsm The GCCGO case should just fail during the configure step now, and the error you reported on FreeBSD should be resolved.

Perfect, I am going to test both scenarios again. 🤝

@thiagoftsm
Copy link
Contributor

After last commits, we do not have more issues with this PR.
I am going to wait it be set as ready for review to retest and approve.

@Ferroin
Copy link
Member Author

Ferroin commented Feb 21, 2024

@Ferroin the integrations script is not working as expected, I am trying to get it working, and make a staging branch so I can get a PR on Learn too that would function properly.

@Ancairon I just cross-checked what the documentation looks like when generated using this branch, and everything looks correct. The collectors page is different because of updates to links to all the Go collectors, and the Go collector docs are getting updated for that and for what appear to be some changes in the agent repo that had not propagated to the Go repo when I copied the code over. The JSON and JS output from the integrations generation also look correct as far as I can tell, so I’m not sure what you’re seeing that’s not working (except possibly the cleanup step, but that does need fixed up for multiple reasons).

@Ancairon
Copy link
Member

I added some commits to this PR and fixed the behavior, yes now it is playing nicely

@Ancairon
Copy link
Member

The MD inside the go directory was not updating before, it was not detected or something like that, but with current state of the file it works

@Ferroin Ferroin marked this pull request as ready for review February 21, 2024 14:04
@Ferroin Ferroin requested a review from ilyam8 February 21, 2024 14:05
@thiagoftsm
Copy link
Contributor

thiagoftsm commented Feb 22, 2024

Hello @Ferroin ,

I retest today on Slackware, and I am having issues again with gccgo:

-- Determining minimum required version of Go for this build
-- Minimum required Go version determined to be 1.21

CMake Error at /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Go: Found unsuitable version "1.18", but required is at
  least "1.21" (found /usr/bin/go)
Call Stack (most recent call first):
  /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:598 (_FPHSA_FAILURE_MESSAGE)
  packaging/cmake/Modules/FindGo.cmake:33 (find_package_handle_standard_args)
  CMakeLists.txt:136 (find_package)


 FAILED  ''

 ABORTED  Failed to configure Netdata sources. 

Another concern that I have happened on a Virtual Machine running Arch Linux:

go: downloading github.com/go-openapi/jsonpointer v0.19.6
go: downloading github.com/josharian/intern v1.0.0
# github.com/jackc/pgx/v4
compile: writing output: write $WORK/b715/_pkg_.a: no space left on device
golang.org/x/net/icmp: write /tmp/go-build1585987896/b752/importcfg: no space left on device
github.com/netdata/go.d.plugin/modules/portcheck: write /tmp/go-build1585987896/b753/importcfg: no space left on device
github.com/netdata/go.d.plugin/modules/powerdns: write /tmp/go-build1585987896/b755/importcfg: no space left on device
github.com/netdata/go.d.plugin/modules/powerdns_recursor: write /tmp/go-build1585987896/b756/importcfg: no space left on device
github.com/netdata/go.d.plugin/modules/prometheus: write /tmp/go-build1585987896/b757/importcfg: no space left on device
github.com/netdata/go.d.plugin/modules/proxysql: write /tmp/go-build1585987896/b758/importcfg: no space left on device

but, I have 6.5 GB free on VM disk. How much space do we need to compile now?

On the other hand, on FreeBSD is working as expected.

@Ferroin
Copy link
Member Author

Ferroin commented Feb 22, 2024

@thiagoftsm The error you’re getting with GCCGO is expected and correct. It only provides Go 1.18 compatibility currently (and not even 100% at that), so it’s not possible for us to use it for a build.

Regarding the space usage aspect, the issue is probably running out of space on tmpfs, not the VM disk itself. I do need to look further into this though.

@thiagoftsm
Copy link
Contributor

@thiagoftsm The error you’re getting with GCCGO is expected and correct. It only provides Go 1.18 compatibility currently (and not even 100% at that), so it’s not possible for us to use it for a build.

The unique real issue, it is the fact I cannot finish compilation. I noticed now that I forgot to add the message:

 FAILED  ''

 ABORTED  Failed to configure Netdata sources. 

I am going to update the original report.

@ilyam8
Copy link
Member

ilyam8 commented Feb 26, 2024

This is the main remaining part of #16808.

It is not mentioned in the issue (there is only "building" which is not enough ofc), but go code testing is missing.

Copy link
Contributor

@thiagoftsm thiagoftsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is working on Slackware, and compilation is going well on FreeBSD, LGTM!

@Ferroin Ferroin merged commit 4b7d920 into netdata:master Feb 26, 2024
123 of 129 checks passed
@Ferroin Ferroin deleted the golang-build branch February 26, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build system (autotools and cmake). area/ci area/collectors Everything related to data collection area/docs area/go area/metadata Integrations metadata area/packaging Packaging and operating systems support area/tests collectors/go.d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants