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

fix(test): Add OpenCL deps to Cache Dependencies workflow #12423

Closed
wants to merge 20 commits into from

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Aug 30, 2024

Related Issues

[Do not merge]

The Cache Dependencies test is failing in #12419.

Checklist

Before you mark the PR ready for review, please make sure that:

@rvagg
Copy link
Member

rvagg commented Sep 2, 2024

Here's what I think is going on:

  • This PR is still failing because you're installing too late, if you moved it up above ~line 219 then it should pass
  • On line 216 we already do this by running .github/actions/install-system-dependencies which has these dependencies on it
  • restore_fetch_params is false because we only care about proof params to determine whether to rebuild or not, only if extern/filecoin-ffi/parameters.json changes do we rebuild.
  • make deps can be run if either restore_fetch_params or restore_make_deps is true, but restore_make_deps is true because it's looking at the extern/filecoin-ffi/ commit hash. But, we don't have dependencies to do this!

I think the fix here is to remove the block you added, but add in this to the if checks for both the install-system-dependencies (line 215) and install-go (line 217) jobs: || steps.restore_make_deps.outputs.cache-hit != 'true'. That way, when we hit make deps because restore_make_deps is true, we have what we need.

@rjan90 rjan90 mentioned this pull request Sep 3, 2024
8 tasks
@BigLep
Copy link
Member

BigLep commented Sep 3, 2024

@rjan90 : I assume we're using this PR to track this issue.

Has all of Rod's feedback/suggestions been incorporated, or is there more we need to here?

@BigLep
Copy link
Member

BigLep commented Sep 3, 2024

@rvagg rvagg force-pushed the phi/test-failing-test branch 2 times, most recently from d0319c0 to b52792a Compare September 4, 2024 05:08
@rjan90 rjan90 force-pushed the phi/test-failing-test branch from 6902973 to 53bab69 Compare September 9, 2024 11:47
chore(ci): capture build logging and CPU flags
.github/workflows/test.yml Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Sep 10, 2024

#12440 seems to address the problem here - instead of using the install-filcrypto script (invoked via the make chain) we use the build-release script in filecoin-ffi directly. Which means that there's some specific build steps in there that aren't available when we use the standard install-filcrypto.

Fixing it this way seems reasonable for now but it's a pretty manual hack for something that we should be able to do with make and an env var or two. So we should figure out how to address it in filecoin-ffi.

rvagg added a commit to filecoin-project/filecoin-ffi that referenced this pull request Sep 10, 2024
Ref: filecoin-project/lotus#12423

Builds using the same flags as rust/scripts/build-release.sh does when called
from CI to build the release bundle.
@rvagg
Copy link
Member

rvagg commented Sep 10, 2024

rvagg added a commit to filecoin-project/filecoin-ffi that referenced this pull request Sep 11, 2024
Ref: filecoin-project/lotus#12423

Builds using the same flags as rust/scripts/build-release.sh does when called
from CI to build the release bundle.
rjan90 and others added 2 commits September 11, 2024 14:11
fix: update account.State import in migration test
* chore(deps): use new FFI_PORTABLE flag for filecoin-ffi builds

* chore: update FFI to v1.30.0-dev

chore: update FFI to v1.30.0-dev

---------

Co-authored-by: Phi <[email protected]>
@rjan90
Copy link
Contributor Author

rjan90 commented Sep 12, 2024

I have fixed the itest-migration failure in 742635a. Only thing missing now is:

@rvagg
Copy link
Member

rvagg commented Sep 12, 2024

You could just fix the codeql check, in multisig/actor.go.template at the top of txnParams:

	if id > uint64(math.MaxInt64) {
		return nil, xerrors.Errorf("transaction ID %d is out of range for int64", id)
	}

it's only used for actors v0 so it's pretty silly to have to mess with this each time, in fact just pulling in msig{{.latestVersion}} for it is silly! maybe we should just push that into message0.go and leave the rest alone

@rvagg
Copy link
Member

rvagg commented Sep 12, 2024

Include #12451 (comment) in here?

nothing to include, that's all done here and that PR can be closed when this is merged

@rvagg
Copy link
Member

rvagg commented Sep 12, 2024

#12454

@rjan90
Copy link
Contributor Author

rjan90 commented Sep 12, 2024

Cleaning up the commits a bit so this can be rebase merged.

I tried to remove a couple of the additions and reversion that were in consecutive commits. But rebasing this PR now is hard because there are recorded submodule modifications which conflicts with the rebase operation. So I think the easier path to get a clean commit history, is to cherry-pick the needed commits over to: #12419

@rjan90 rjan90 mentioned this pull request Sep 12, 2024
8 tasks
@rjan90
Copy link
Contributor Author

rjan90 commented Sep 12, 2024

Closing as the relevant changes here has been added/cherry-picked into: #12455

@rjan90 rjan90 closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

4 participants