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

Add build flags to make CGO optional #1094

Merged
merged 4 commits into from
Oct 25, 2019
Merged

Add build flags to make CGO optional #1094

merged 4 commits into from
Oct 25, 2019

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Oct 22, 2019

The ZSTD library requires CGO. This commit adds build flags to ignore
ZSTD if CGO is disabled.

To build badger with ZSTD (and CGO) set CGO_ENABLED=1
To build badger without ZSTD (and CGO) set CGO_ENABLED=0


This change is Reviewable

The ZSTD library requires CGO. This commit adds build flags to ignore
ZSTD if CGO is disabled.

To build badger with ZSTD (and CGO) set CGO_ENABLED=1
To build badger without ZSTD (and CGO) set CGO_ENABLED=0
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@jarifibrahim you can click here to see the review status or cancel the code review job.

@coveralls
Copy link

coveralls commented Oct 22, 2019

Coverage Status

Coverage decreased (-0.2%) to 77.409% when pulling a572e60 on ibrahim/zstd-cgo into be6402f on master.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Just one comment on the file name.


Reviewed with ❤️ by PullRequest

table/zstd_fallback.go Outdated Show resolved Hide resolved
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm: other than a minor fix.

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, @manishrjain, and @pullrequest[bot])


table/zstd_nocgo.go, line 25 at r2 (raw file):

)

var errZstdCgo = errors.New("zstd compression requires building with cgo enabled")

"zstd compression requires building badger with cgo enabled"

Add a mention of badger, otherwise users of other software that has badger as a dependency might be confused.

Copy link
Contributor Author

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @manishrjain, @martinmr, and @pullrequest[bot])


table/zstd_nocgo.go, line 25 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

"zstd compression requires building badger with cgo enabled"

Add a mention of badger, otherwise users of other software that has badger as a dependency might be confused.

Thanks! I've changed it.

kung-foo added a commit to Intelecy/badger that referenced this pull request Oct 23, 2019
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @martinmr, and @pullrequest[bot])

options.go Show resolved Hide resolved
@jarifibrahim jarifibrahim merged commit 11bfbfd into master Oct 25, 2019
@jarifibrahim jarifibrahim deleted the ibrahim/zstd-cgo branch October 25, 2019 06:59
@zanicar
Copy link

zanicar commented Oct 29, 2019

Unfortunately go get still fails as a result of the gcc dependency. I can't build what I don't have...

@jarifibrahim
Copy link
Contributor Author

Unfortunately go get still fails as a result of the gcc dependency. I can't build what I don't have...

Oh, right. go get ... would try to build ZSTD by default. Let me see what we can do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants