Skip to content

Makefiles: Disable CGO globally#10724

Merged
aanm merged 1 commit intocilium:masterfrom
joestringer:submit/fix-broken-master
Apr 2, 2020
Merged

Makefiles: Disable CGO globally#10724
aanm merged 1 commit intocilium:masterfrom
joestringer:submit/fix-broken-master

Conversation

@joestringer
Copy link
Copy Markdown
Member

@joestringer joestringer commented Mar 26, 2020

CGo is not necessary for any Cilium binaries (only proxylib), and with
the new docs builder image it's causing build issues like the following
on master for some developers:

  GEN   Documentation/cmdref
Error relocating /src/cilium/cilium: __vfprintf_chk: symbol not found
Error relocating /src/cilium/cilium: __fprintf_chk: symbol not found

Fix this by just disabling it for all targets that use $GO.

@joestringer joestringer added pending-review priority/high This is considered vital to an upcoming release. release-note/misc This PR makes changes that have no direct user impact. labels Mar 26, 2020
@joestringer joestringer requested a review from a team as a code owner March 26, 2020 18:44
@joestringer
Copy link
Copy Markdown
Member Author

test-me-please

@joestringer
Copy link
Copy Markdown
Member Author

I guess we use it for proxylib, will need to fix that up..

@joestringer joestringer force-pushed the submit/fix-broken-master branch from 66addcd to 4b03a76 Compare March 26, 2020 19:01
@joestringer
Copy link
Copy Markdown
Member Author

joestringer commented Mar 26, 2020

test-me-please

Edit: Kafka flakes; fix has been merged: #10721

@christarazi
Copy link
Copy Markdown
Member

test-me-please

Copy link
Copy Markdown
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

I think you'll need to drop CGO_ENABLED=0 from the go vet commands for proxylib. See the Travis CI failure.

@joestringer joestringer force-pushed the submit/fix-broken-master branch from 4b03a76 to ef582ad Compare March 26, 2020 23:54
@joestringer
Copy link
Copy Markdown
Member Author

test-me-please

@joestringer
Copy link
Copy Markdown
Member Author

Oh, originally I thought it was go vet but it looks like it's actually the go tests in the proxylib directory.

@joestringer joestringer force-pushed the submit/fix-broken-master branch from ef582ad to 0c7f9df Compare March 27, 2020 02:19
@joestringer
Copy link
Copy Markdown
Member Author

test-me-please

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 27, 2020

Coverage Status

Coverage remained the same at 45.517% when pulling 5a264bc on joestringer:submit/fix-broken-master into 26dec4c on cilium:master.

@joestringer joestringer added wip and removed pending-review priority/high This is considered vital to an upcoming release. labels Mar 27, 2020
@joestringer
Copy link
Copy Markdown
Member Author

Plenty of scary-looking failures in CI, will need to investigate further:
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18287/

CGo is not necessary for any Cilium binaries, and with the new docs
builder image it's causing build issues like the following:

      GEN   Documentation/cmdref
    Error relocating /src/cilium/cilium: __vfprintf_chk: symbol not found
    Error relocating /src/cilium/cilium: __fprintf_chk: symbol not found

Fix this by just disabling it for all targets that use $GO.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer force-pushed the submit/fix-broken-master branch from 0c7f9df to 5a264bc Compare April 1, 2020 22:53
@joestringer
Copy link
Copy Markdown
Member Author

joestringer commented Apr 1, 2020

test-me-please

EDIT: Only one of the kernel builds failed, due to flake #10821 which looks like infrastructure issue.

@joestringer joestringer added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed wip labels Apr 2, 2020
@aanm aanm merged commit 7626be1 into cilium:master Apr 2, 2020
@joestringer joestringer deleted the submit/fix-broken-master branch May 22, 2020 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants