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

Ensure every plugins can build #109

Merged
merged 8 commits into from
Aug 21, 2021

Conversation

darkweak
Copy link
Owner

Closes #98

@darkweak darkweak force-pushed the fix/plugins/caddy/ensure-caddy-module-can-be-built branch 13 times, most recently from 6da6d8b to 4f64d7a Compare August 18, 2021 20:27
@darkweak darkweak force-pushed the fix/plugins/caddy/ensure-caddy-module-can-be-built branch from 4f64d7a to 942489c Compare August 18, 2021 21:07
@darkweak darkweak force-pushed the fix/plugins/caddy/ensure-caddy-module-can-be-built branch from dec71ed to 02a934d Compare August 18, 2021 21:12
@darkweak darkweak changed the title Ensure plugins can be built Ensure every plugins can build Aug 18, 2021
Copy link

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

If possible to use the newer v3 release of Badger that would be good. I was under the impression the build failure was due to a dependency on an RC v2 version of Badger (which is apparently from smallstep/nosql).

I believe the issue was actually resolved when you updated dependencies as just 2 days ago, smallstep/nosql updated that RC v2 dependency to the latest v2 release. Perhaps downgrading the version of v3 for you was just coincidence in timing and mislead you as the solution? (EDIT: May still take a little longer as Caddy has yet to update to that newer version (PR), and of course make a release)

Comment on lines +7 to +8
github.com/dgraph-io/badger/v3 v3.2011.1
github.com/dgraph-io/ristretto v0.0.4-0.20210122082011-bb5d392ed82d

Choose a reason for hiding this comment

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

Downgrading Badger v3 prevents Caddy from building? That isn't due to the prior commits build failure was it? That was about an RC version of Badger v2 AFAIK.

At least when I used xcaddy I used --with to override for latest v2 release and it build successfully avoiding the same build failure error I linked which was experienced in my bug report.

There was some nice improvements in the newer releases of Badger v3, if there was another issue that you experienced with building Caddy can you reference it and any related Github issue to track?

Comment on lines +3 to +4
github.com/DataDog/zstd v1.4.1 h1:3oxKN3wbHibqx897utPC2LTQU4J+IHWWJO+glkAkpFM=
github.com/DataDog/zstd v1.4.1/go.mod h1:1jcaCB/ufaK+sKp1NBhlGmpz41jOoPQ35bpF36t7BBo=
Copy link

@polarathene polarathene Aug 18, 2021

Choose a reason for hiding this comment

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

If upgrading to newer Badger v3, there is a native Go version of zstd to replace the datadog CGo dependency.

Caddy will still depend on the datadog one due to dependency on smallstep/nosql having a upgrade blocker to Badger v3, but a Caddy maintainer has set up a PR for Badger v2 to backport this zstd dependency change. (EDIT: Not sure about the datadog dependency for Caddy, since Caddy apparently builds with CGo disabled)

Copy link

@francislavoie francislavoie Aug 19, 2021

Choose a reason for hiding this comment

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

BTW, go.sum is not dependencies, it's just all the collected hashes of sources in the dependency graph. It doesn't actually mean that version will be used in the build.

The go.sum file may contain hashes for multiple versions of a module. The go command may need to load go.mod files from multiple versions of a dependency in order to perform minimal version selection.

https://golang.org/ref/mod#go-sum-files

Copy link

@polarathene polarathene Aug 19, 2021

Choose a reason for hiding this comment

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

BTW, go.sum is not dependencies, it's just all the collected hashes of sources in the dependency graph.

Thanks for the clarification! I'm a Go noob 😬

It doesn't actually mean that version will be used in the build.

Is it building a dependency when I get errors like this from xcaddy?:

/go/pkg/mod/github.com/dgraph-io/badger/v2@v2.0.1-rc1.0.20201003150343-5d1bab4fc658/table/table.go:849:18: not enough arguments in call to z.Calloc   

Updating that RC version to newer using xcaddy --with resolved it. AFAIK that version of badger is/was a dependency of smallstep/nosql that was recently updated and Caddy will update as well.

Presumably it was a minimal version for v2 because of the RC suffix, so semver rules to use a newer version were invalid? (that or there was no other Badger v2 versions referenced so it settled on that one? 🤷‍♂️ )

The go.sum is like yarn.lock/cargo.lock from what you've described. Will it be stale and still reference the wrong version causing a build error after Caddy updates? (assuming that the go.sum for this module will need to be updated, even if go.mod is updated or I use xcaddy --with for the newer Caddy release)

Hopefully this PR is enough to prevent the build issues in future 😅

Choose a reason for hiding this comment

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

Is it building a dependency when I get errors like this from xcaddy?:

smallstep/nosql just happened to have last updated its badger dependency to that v2.0.1-rc1 release, so that's what was used. I don't know why that particular error was happening. Too deep down the chain, and I don't super care to dig to find out. I just want the CGO dependencies gone ASAP 😬 but the recent version bump to the latest v2 of badger should be a decent stopgap even if my PR for backporting the removal of DataDog/zstd isn't merged anytime soon.

The go.sum is like yarn.lock/cargo.lock from what you've described.

That's essentially my understanding, yeah. Go stores the actual sources in your GOPATH somewhere, and those versions and hashes are references to specific versions, so Go can verify that what's in the go.sum matches what was downloaded out of band.

Will it be stale and still reference the wrong version causing a build error after Caddy updates?

As the link I sent above explains, any go get or go mod tidy command updates the go.sum automatically, so it's typically kept up to date pretty well.

@darkweak darkweak force-pushed the fix/plugins/caddy/ensure-caddy-module-can-be-built branch 7 times, most recently from a1ab6a5 to d422f41 Compare August 21, 2021 13:12
@darkweak darkweak force-pushed the fix/plugins/caddy/ensure-caddy-module-can-be-built branch 21 times, most recently from 68ef926 to 157ae7e Compare August 21, 2021 15:21
@darkweak darkweak force-pushed the fix/plugins/caddy/ensure-caddy-module-can-be-built branch 2 times, most recently from a44cd73 to ee22f49 Compare August 21, 2021 15:44
@darkweak darkweak force-pushed the fix/plugins/caddy/ensure-caddy-module-can-be-built branch from ee22f49 to d071f73 Compare August 21, 2021 15:46
@darkweak
Copy link
Owner Author

I'll merge without tag a new version and (me or someone else) will open another PR to bump deps when the one on the badger repository will be merge on it.

@darkweak darkweak merged commit 6af3835 into master Aug 21, 2021
@darkweak darkweak deleted the fix/plugins/caddy/ensure-caddy-module-can-be-built branch August 24, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to build Caddy plugin
3 participants