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

go.mod, build: upgrade c-kzg-4844 #27907

Merged
merged 8 commits into from
Aug 11, 2023
Merged

go.mod, build: upgrade c-kzg-4844 #27907

merged 8 commits into from
Aug 11, 2023

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Aug 11, 2023

This upgrades to the latest release of ckzg, and also attempts to fix some blst-related
build errors that occur on launchpad.net.

go.sum Outdated
github.com/modocache/gover v0.0.0-20171022184752-b58185e213c5/go.mod h1:caMODM3PzxT8aQXRPkAt8xlV/e7d7w8GM5g0fa5F0D8=
github.com/moul/http2curl v1.0.0/go.mod h1:8UbvGypXm98wA/IqH45anm5Y2Z6ep6O31QGOAZ3H0fQ=
github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=
Copy link
Contributor

Choose a reason for hiding this comment

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

How can c-kzg affect go-conntrack? Are you sure this is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ran go mod tidy

@@ -55,7 +55,7 @@ func (g *GoToolchain) Go(command string, args ...string) *exec.Cmd {
}
// CKZG by default is not portable, append the necessary build flags to make
// it not rely on modern CPU instructions and enable linking against
tool.Env = append(tool.Env, "CGO_CFLAGS=-D__BLST_PORTABLE__")
tool.Env = append(tool.Env, "CGO_CFLAGS=-std=c99 -O2 -D__BLST_PORTABLE__")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

holiman
holiman previously approved these changes Aug 11, 2023
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@fjl fjl dismissed holiman’s stale review August 11, 2023 15:37

It's not working

@fjl
Copy link
Contributor Author

fjl commented Aug 11, 2023

Not sure why, but changing CGO_CFLAGS like that seems to create more problems than it solves.

@jtraglia
Copy link
Member

Just FYI, a new version of c-kzg-4844 (v0.3.1) was just released:

@@ -55,7 +55,7 @@ func (g *GoToolchain) Go(command string, args ...string) *exec.Cmd {
}
// CKZG by default is not portable, append the necessary build flags to make
// it not rely on modern CPU instructions and enable linking against
tool.Env = append(tool.Env, "CGO_CFLAGS=-D__BLST_PORTABLE__")
tool.Env = append(tool.Env, "CGO_CFLAGS=-std=c11 -O2 -D__BLST_PORTABLE__")
Copy link
Member

@jtraglia jtraglia Aug 11, 2023

Choose a reason for hiding this comment

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

You need to surround the values in quotation marks.

Suggested change
tool.Env = append(tool.Env, "CGO_CFLAGS=-std=c11 -O2 -D__BLST_PORTABLE__")
tool.Env = append(tool.Env, "CGO_CFLAGS=\"-std=c11 -O2 -D__BLST_PORTABLE__\"")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's required here. The process environment is a slice of strings and everything after the first = is the variable value. The env slice is not interpreted by shell.

Copy link
Member

Choose a reason for hiding this comment

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

Oh gotcha. I didn't know that. You're right.

@fjl
Copy link
Contributor Author

fjl commented Aug 11, 2023

Submitted a different approach for fixing the Launchpad build now, just disable ckzg on Ubuntu Trusty where the build fails.

@fjl fjl added this to the 1.12.2 milestone Aug 11, 2023
@fjl fjl changed the title go.mod: upgrade c-kzg-4844 go.mod, build: upgrade c-kzg-4844 Aug 11, 2023
@fjl fjl merged commit e91b21c into ethereum:master Aug 11, 2023
1 of 2 checks passed
fjl added a commit that referenced this pull request Aug 12, 2023
This upgrades to the latest release of ckzg, and also attempts to fix some blst-related
build errors that occur on launchpad.net.
MoonShiesty pushed a commit to MoonShiesty/go-ethereum that referenced this pull request Aug 30, 2023
This upgrades to the latest release of ckzg, and also attempts to fix some blst-related
build errors that occur on launchpad.net.
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This upgrades to the latest release of ckzg, and also attempts to fix some blst-related
build errors that occur on launchpad.net.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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.

None yet

3 participants