Skip to content

op-node: remove unnecessary cgo binding in tests#10825

Merged
sebastianst merged 1 commit intoethereum-optimism:developfrom
gosunuts:feat/zstd-cgo-remove
Jun 14, 2024
Merged

op-node: remove unnecessary cgo binding in tests#10825
sebastianst merged 1 commit intoethereum-optimism:developfrom
gosunuts:feat/zstd-cgo-remove

Conversation

@gosunuts
Copy link
Contributor

@gosunuts gosunuts commented Jun 14, 2024

Description
remove unnecessary cgo binding in test.

Additional context
cgo makes build more complicated and disable cross-compile.
I think it is not good idea using cgo binding without any discussion.

Metadata

#10750
#10358
https://dave.cheney.net/2016/01/18/cgo-is-not-go
dgraph-io/badger#1162 - Use pure Go zstd implementation

@ajsutton
Copy link
Contributor

@sebastianst I think you probably have the most context on whether we need a specific zstd library or not for this test, can you take a look.

I thought we had cgo dependencies via things like op-geth anyway to be honest, but not sure.

@ajsutton ajsutton requested a review from sebastianst June 14, 2024 12:19
@gosunuts
Copy link
Contributor Author

@sebastianst I think you probably have the most context on whether we need a specific zstd library or not for this test, can you take a look.

I thought we had cgo dependencies via things like op-geth anyway to be honest, but not sure.

As my experience, using cgo unnecessarily complicates whole project. It's best to avoid it as much as possible.

@ajsutton
Copy link
Contributor

It may well be unavoidable for some of the precompile dependencies on the execution side.

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

@ajsutton the switch is fine here, because this is just a test for invalid compression anyways

@sebastianst sebastianst enabled auto-merge June 14, 2024 14:07
@sebastianst sebastianst added this pull request to the merge queue Jun 14, 2024
Merged via the queue into ethereum-optimism:develop with commit b76fd74 Jun 14, 2024
rdovgan pushed a commit to rdovgan/optimism that referenced this pull request Jun 24, 2024
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.

3 participants