fix: failed to build binary by make retina-binary

[nayihz]

Addresses issue #128

[nayihz]

@microsoft-github-policy-service agree

[rbtr]

@nayihz while I believe that this fixes the issue, I don't understand why we don't encounter this issue when we build in our container? https://github.com/microsoft/retina/blob/main/controller/Dockerfile.controller#L38

[timraymond]

TIL about //go:build ignore, thanks for the contribution 😄 This looks pretty good to me as-is... I was able to build this with make retina-binary without any additional ENVs (we had previously been setting CGO_ENABLED=0, but this seems better). I did find that there's a few object files left behind from compiling BPF... would you be interested in adding those to .gitignore as well? They are:

• pkg/plugin/dropreason/kprobe_bpfel_x86.o
• pkg/plugin/filter/filter_bpfel_x86.o
• pkg/plugin/packetforward/packetforward_bpfel_x86.o
• pkg/plugin/packetparser/packetparser_bpfel_x86.o

[timraymond]

@rbtr It's because we set CGO_ENABLED=0 here:

retina/controller/Dockerfile.controller

Line 30 in cf23ecb

ENV CGO_ENABLED=0

. The go toolchain skips over its distaste for C here when CGO is disabled: https://github.com/golang/go/blob/e7bdc8819a19f06321d300719224abb2d8567b4a/src/cmd/go/internal/load/pkg.go#L2060-L2063

[rbtr]

@rbtr It's because we set CGO_ENABLED=0 here:

@timraymond We also disable CGO in the Make target to build the binary, though:

retina/Makefile

Lines 148 to 149 in cf23ecb

	export CGO_ENABLED=0
	go build -v -o $(RETINA_BUILD_DIR)/retina$(EXE_EXT) -gcflags="-dwarflocationlists=true" -ldflags "-X main.version=$(TAG) -X main.applicationInsightsID=$(APP_INSIGHTS_ID)" $(RETINA_DIR)/main.go

[rbtr]

Actually if I'm reading your link correctly
len(p.CFiles) > 0 && !p.UsesCgo() means that we get this error when CGO is disabled and there are C files present, right? So the unexpected behavior seems to be that in the Dockerfile, CGO remains enabled despite the env var being set, leading to a sort of "false positive" successful build?

[nayihz]

@rbtr It's because we set CGO_ENABLED=0 here:

@timraymond We also disable CGO in the Make target to build the binary, though:

retina/Makefile

Lines 148 to 149 in cf23ecb

	export CGO_ENABLED=0
	go build -v -o $(RETINA_BUILD_DIR)/retina$(EXE_EXT) -gcflags="-dwarflocationlists=true" -ldflags "-X main.version=$(TAG) -X main.applicationInsightsID=$(APP_INSIGHTS_ID)" $(RETINA_DIR)/main.go

export CGO_ENABLED=0 doesn't seem to take effect in makefile. We can build successfully when we add export CGO_ENABLED=0 before go generate command.

# makefile
retina-binary: ## build the Retina binary
        export CGO_ENABLED=0 && \
	go generate ./... && \
        go build -v -o $(RETINA_BUILD_DIR)/retina$(EXE_EXT) -gcflags="-dwarflocationlists=true" -ldflags "-X main.version=$(TAG) -X main.applicationInsightsID=$(APP_INSIGHTS_ID)" $(RETINA_DIR)/main.go

[nayihz]

Modifed the makefile to enable CGO_ENABLED=0 and omit ".o" files.
PTAL.

[rbtr]

I'm afraid we need those .o files:

Error: 10.40 pkg/plugin/filter/filter_bpfel_arm64.go:119:12: pattern filter_bpfel_arm64.o: no matching files found
Error: 10.40 pkg/plugin/dropreason/kprobe_bpfel_arm64.go:190:12: pattern kprobe_bpfel_arm64.o: no matching files found
Error: 10.40 pkg/plugin/packetforward/packetforward_bpfel_arm64.go:125:12: pattern packetforward_bpfel_arm64.o: no matching files found
Error: 10.40 pkg/plugin/packetparser/packetparser_bpfel_arm64.go:159:12: pattern packetparser_bpfel_arm64.o: no matching files found

https://github.com/microsoft/retina/actions/runs/8429451457/job/23089687153?pr=129

[anubhabMajumdar]

Thanks for the PR. Can you please remove the generated object files (not delete them from the repository, just the updated ones from your PR)? I think they don't need to be updated as part of the PR.

[rbtr]

@nayihz can you please sign and sign-off your commits? git commit -S -s --amend --no-edit should do it

[nayihz]

@nayihz can you please sign and sign-off your commits? git commit -S -s --amend --no-edit should do it

Done.

[rbtr]

Sorry @nayihz, you have correctly Signed-off-By in the commit message message, but it looks like you're missing the cryptographic signature on the commit itself.

This Verified in the commits page is what I'm looking for:

It is a merge requirement that all commits be signed.
See #197

[nayihz]

This Verified in the commits page is what I'm looking for:

Done. PTAL.

[rbtr]

looks good, thanks!