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

Disable cgo on release builds #365

Merged

Conversation

tomelliff
Copy link
Contributor

@tomelliff tomelliff commented Jan 28, 2020

Description

Cgo tries to link against libc which isn't present in musl based distributions (such as Alpine Linux).

Motivation and Context

#329

How Has This Been Tested?

  • make release
  • readelf -l release/oauth2_proxy-v4.1.0-37-gd9362d3-dirty.linux-amd64.go1.13.1/oauth2_proxy | grep "program interpreter"
  • docker run -it --rm -v $PWD:/work -w /work alpine ./release/oauth2_proxy-v4.1.0-37-gd9362d3-dirty.linux-amd64.go1.13.1/oauth2_proxy --help

I don't have a non Linux machine to test this on but I don't see why this would be broken on any other arch/os. The v4.0.0 release used to disable cgo until the dist.sh rewrite in #302 which I think inadvertently reintroduced it.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

Tries to link against libc which isn't present in musl based distributions (such as Alpine Linux).

Closes oauth2-proxy#329
@tomelliff
Copy link
Contributor Author

I don't think this needs a changelog entry because it's just fixing the build process and isn't user facing really but let me know if you want me to add something anyway.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Thanks @tomeliff for playing about with this and verifying this fixes the issue. I was pretty sure we had had the CGO_ENABLED=0 option before but as you say, it got lost!

As people have had issues running this on alpine with the latest release, I would suggest this is a bug fix and that a changelog entry should be added

@syscll, since you've modified these files recently, can you see any issue adding this back?

Copy link
Member

@loshz loshz left a comment

Choose a reason for hiding this comment

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

LGTM - I also think we should add a CHANGELOG entry.

@starkers starkers merged commit 5c8220d into oauth2-proxy:master Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants