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

chore : CoreTypes : Remove stray C dependency #12015

Merged
merged 1 commit into from
May 23, 2024

Conversation

ribasushi
Copy link
Collaborator

Without this change the following test fails:

CGO_ENABLED=0 go test -count=1 ./chain/types_test.go

Without this fix importing the chain types into a fully-static executable is not possible.

I am not adding a test, as that should be done within the CI harness somehow (there is no way to force-disable compile-time flags from a test itself). Someone more familiar with how tests are run on a macro-level (@rvagg ?) should add a one-liner somewhere so things do not regress in the future.

@ribasushi
Copy link
Collaborator Author

Failure is a flaky itest (10m timeout). The new repo perms do not allow externals to assign revewers, so: @ZenGround0 @Stebalien @rvagg

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

🤷 I don't have any bright ideas for locking in testing that would prevent regressions, we have unit tests all over the place that need cgo so it can't just be a re-run with a blanket exclusion, it'd have to be specific.

@rvagg
Copy link
Member

rvagg commented May 21, 2024

@ribasushi how about removing the dependency completely so it'll be obvious if it ever comes back?

build/builtin_actors_gen_test.go still has a reference, which seems a bit redundant since we're now using 2 libraries for the same code paths .. perhaps it helps test correctness but do we think there's a problem?

cmd/lotus/daemon.go of course is the big one, it'd be interesting to get an idea of the speed difference importing a snapshot between the two implementations, if it's nearly as good, or better, then we may as well ditch the dependency entirely?

@ribasushi
Copy link
Collaborator Author

ribasushi commented May 21, 2024

I don't have any bright ideas for locking in testing that would prevent regressions ... it can't just be a re-run with a blanket exclusion

@rvagg I meant a sub-runner for CI that re-executes just a portion of tests which specificaly deal with types. Not unlike the make unittests thing you made. As a start it could be as simple as what I wrote above: a list with a single entry chain/types_test.go. What I don't know is how everything is wired with CircleCI (or is it GHA now?).

@Stebalien
Copy link
Member

@rvagg unfortunately, badger v2 pulls in github.com/DataDog/zstd. So we're not going to be able to completely ditch it. But:

  1. I'd definitely remove it from builtin_actors_gen_test.go.
  2. I'd leave it in cmd/lotus/daemon.go unless someone wants to benchmark snapshot imports (snapshot importing may be io bound, but I'm not sure).

@rvagg
Copy link
Member

rvagg commented May 23, 2024

@ribasushi would you mind at least removing the test dependency?

Without this change the followung test fails:

CGO_ENABLED=0 go test -count=1 ./chain/types_test.go
@ribasushi ribasushi force-pushed the chore/types_no_cgo branch from 4cca36a to 0bb0503 Compare May 23, 2024 07:39
@ribasushi
Copy link
Collaborator Author

@rvagg @Stebalien removed zstd from test. Though note that this is not about zstd itself at all.

If one wants to do anything at all with the chain, there are some types that can't be avoided ( TipSetKey, TipSet, etc ). All these "chain types" live in lotus, not in go-state-types.

Infecting this part of the codebase with any CGO dep is what the issue is. This is why I keep asking for advice how to ensure that the type tests run under CI with and without CGO

@rvagg
Copy link
Member

rvagg commented May 23, 2024

We could build this into the test.yml workflow - add another utests item with a specific list and add a new property like yugabytedb and parameters to inject the cgo disable env var. But ./chain/types_test.go doesn't buy you much, just Message and SignedMessage, right?

If you want to make a list of tests to run I can try and work on something but there's just not much to ./chain/types_test.go that it doesn't seem worth the hassle to me.

@rvagg rvagg merged commit 9391548 into filecoin-project:master May 23, 2024
94 checks passed
@ribasushi ribasushi deleted the chore/types_no_cgo branch May 23, 2024 14:52
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