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: merge v1.28.1 release back to master & update master version to v1.28.2-dev #12306

Closed
wants to merge 19 commits into from

Conversation

jennijuju
Copy link
Member

No description provided.

LexLuthr and others added 19 commits July 25, 2024 01:46
* remove provecommit1

* add changelog

* update precommit and commit params

* fix lint error

* fix commit params
* Update go-f3 to 0.0.6

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

* Enable F3 in passive configuration in mainnet config

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

* Add changelog

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

* add new butterfly assets

---------

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Co-authored-by: Jennifer Wang <jiayingw703@gmail.com>
We now use the same one in GPBFT.
Co-authored-by: Steven Allen <steven@stebalien.com>
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Rod Vagg <rod@vagg.org>
@jennijuju jennijuju requested a review from rvagg July 25, 2024 06:08
@ribasushi
Copy link
Collaborator

🚨 the FFI does not agree on both ends

~$ git diff v1.28.1..origin/jen/v1.28.1releasetomaster extern
diff --git a/extern/filecoin-ffi b/extern/filecoin-ffi
index e467d2992..d041c9ab8 160000
--- a/extern/filecoin-ffi
+++ b/extern/filecoin-ffi
@@ -1 +1 @@
-Subproject commit e467d2992e3f9bd09beb71ecf84323b45d2a3511
+Subproject commit d041c9ab85e68cfb57daf5fbaedeeee0f72cb5ad

neither does the current one in master

~$ git diff origin/master..v1.28.1 extern
diff --git a/extern/filecoin-ffi b/extern/filecoin-ffi
index d041c9ab8..e467d2992 160000
--- a/extern/filecoin-ffi
+++ b/extern/filecoin-ffi
@@ -1 +1 @@
-Subproject commit d041c9ab85e68cfb57daf5fbaedeeee0f72cb5ad
+Subproject commit e467d2992e3f9bd09beb71ecf84323b45d2a3511

@rvagg
Copy link
Member

rvagg commented Jul 25, 2024

mmm, #12271 includes the commit (PR) that was supposed to change it #12276 but it seems to have got lost for the 1.28.0 final. So releases and the 1.28.x releases have e467d2992e3f9bd0 but master has d041c9ab85e68cfb

Thankfully the difference is minimal and not material: filecoin-project/filecoin-ffi@e467d29...d041c9a

But master is on d041c9ab85e68cfb, which is https://github.com/filecoin-project/filecoin-ffi/releases/tag/v1.28.0, which is what we want. We just happened to release 1.28.0 and 1.28.1 with https://github.com/filecoin-project/filecoin-ffi/releases/tag/v1.28.0-rc2

A concern, and another reason we need to bail on the complicated merge, mega-squash, multi-branch dance I think. But not fatal to the releases. We have the right ref-fvm and proofs versions.

@rjan90
Copy link
Contributor

rjan90 commented Jul 25, 2024

🚨 the FFI does not agree on both ends

  1. d041c9ab85e68cfb57daf5fbaedeeee0f72cb5ad corresponds to https://github.com/filecoin-project/filecoin-ffi/releases/tag/v1.28.0.
  2. e467d2992e3f9bd09beb71ecf84323b45d2a3511 corresponds to https://github.com/filecoin-project/filecoin-ffi/releases/tag/v1.28.0-rc2

Chain of events:

  1. d041c-FFI updated in master here chore: deps: Update GST, Filecoin-FFI and Actors to final versions NV23 #12276
  2. The chore: deps: Update GST, Filecoin-FFI and Actors to final versions NV23 #12276 PR was listed and backported to the release/v1.28.0 branch as part of the final prep: 5afd10a. But the extern/filecoin-ffi change did not get included in that backport.

Both FFI-versions are good for nv23, and the e467d-FFI (v1.28.0-rc2) is the one that was used during Calibration network upgrade, and no changes related to nv23 has landed between those versions as seen by the v1.28.0-rc2...v1.28.0.

That said, it's not good that I missed that extern/filecoin-ffi was not included in the backport for the final prep of v1.28.0. And that we did not caught it during review. I will add it as an item during nv23-retro and discussions on branch names/merges we use, as this would have been bad in the case where the Filecoin-FFI actually needed change.

  • One initial though here is having a GHA-bot that comments if there is a diff between the extern/filecoin-ffi hash in their PR, and the one in the master branch.

@rvagg
Copy link
Member

rvagg commented Jul 25, 2024

(and this PR is correct to not pull a change back in btw)

@rvagg
Copy link
Member

rvagg commented Jul 25, 2024

Oh, and yet another reason to get the ffi prebuilds work done!

@BigLep
Copy link
Member

BigLep commented Jul 25, 2024

Concerning FFI not agreeing on both ends and the explanation, this is inevitable I think with our consistent setup. Items around standardizing how we commit, standardizing how we backport, etc. are observations I've been jotting down in https://docs.google.com/document/d/11jN9E4IcgcbU_6acAIHuh29v7yGFj7OHiiGYpoUuSEs/edit?pli=1#heading=h.qkulnat42emv . Others feel free to put in lessons learned as well. These seem like some of the quickest low hanging fruit to address after the network upgrade.

I also in future would expect to see a clearer commit history. I think ideally every (or close to every) commit would list its assocated PR in the commit message. (We get some of that by default if there is more squash and merge happening.)

@rvagg rvagg mentioned this pull request Jul 29, 2024
@rvagg
Copy link
Member

rvagg commented Jul 29, 2024

done in #12317

@rvagg rvagg closed this Jul 29, 2024
@rvagg rvagg deleted the jen/v1.28.1releasetomaster branch July 29, 2024 07:25
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.

8 participants