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: Updating nim-chronicles, nim-chronos, nim-presto, nimcrypto, nim-libp2p, and nim-nat-transversal #2043

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

Ivansete-status
Copy link
Collaborator

Description

This is a maintenance PR, and the most relevant vendor dependencies are being updated. Not all of the dependencies are updated to try to reduce the number of candidates, in case of any error.

Changes

  • nim-chronicles, nim-chronos, nim-presto, nimcrypto, zerokit, nim-libp2p, and nim-nat-transversal updated.

Issue

#2041

nimcrypto, zerokit, nim-libp2p, and nim-nat-transversal
@Ivansete-status Ivansete-status self-assigned this Sep 18, 2023
@rymnc
Copy link
Contributor

rymnc commented Sep 18, 2023

Hi, imo it is not necessary to update the zerokit submodule, we're trying to freeze nwaku usage to v0.3.4

@github-actions
Copy link

github-actions bot commented Sep 18, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2043

Built from dd9dcac

@Ivansete-status Ivansete-status marked this pull request as ready for review September 18, 2023 14:30
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Happy with this strategy, except that painful as it may be, wouldn't it in the long run be easier/safer to update all submodules? Updating only a subgroup you run the risk of unexpected behaviour in other submodules that may rely on the updated modules? No blocker here to merge, but a thought for future iterations.

@SionoiS
Copy link
Contributor

SionoiS commented Sep 18, 2023

Happy with this strategy, except that painful as it may be, wouldn't it in the long run be easier/safer to update all submodules? Updating only a subgroup you run the risk of unexpected behaviour in other submodules that may rely on the updated modules? No blocker here to merge, but a thought for future iterations.

We could update all submodules (with exceptions) before a release, make it part of the process? Or post release?

edit: seams we already do?

@jm-clius
Copy link
Contributor

edit: seams we already do?

It's part of the process here: https://github.com/waku-org/nwaku/blob/master/docs/contributors/release-process.md#before-release but I think we've skipped doing it for a couple of releases

@Ivansete-status
Copy link
Collaborator Author

Happy with this strategy, except that painful as it may be, wouldn't it in the long run be easier/safer to update all submodules?

Good point! My plan is to first upgrade nim-chronos and nim-libp2p as I consider them to be the most important. Then, check that everything works correctly during some days in wakuv2.test. That approach wanted to pinpoint clearly cases when a mem leak arises, although this can be tackled properly with heaptrack.

Updating only a subgroup you run the risk of unexpected behaviour in other submodules that may rely on the updated modules?

Yes, you are absolutely right! I'm assuming that, if all compiles, then no issues should happen. However, all submodules should be updated before the next release to minimize the risk, as you pointed out :).

I suggest making this "incremental" upgrade now, and if we encounter issues, then to all at once the next time.

@Ivansete-status
Copy link
Collaborator Author

edit: seams we already do?

It's part of the process here: https://github.com/waku-org/nwaku/blob/master/docs/contributors/release-process.md#before-release but I think we've skipped doing it for a couple of releases

Honestly, I vote for upgrading right after the release. That way, we have more time to validate the correctness of such an upgrade. I think is good to have it as a regular post-build task. wdyt ?

@gabrielmer
Copy link
Contributor

Happy with this strategy, except that painful as it may be, wouldn't it in the long run be easier/safer to update all submodules? Updating only a subgroup you run the risk of unexpected behaviour in other submodules that may rely on the updated modules? No blocker here to merge, but a thought for future iterations.

Maybe the updates can be done in groups of inter-dependent modules? Most likely it wouldn't help a lot I think, but it might be worth thinking about it

@jm-clius
Copy link
Contributor

I vote for upgrading right after the release

Indeed. I think the spirit of this item in the release process is to ensure that it was upgraded during each release cycle. If we do it at the beginning of the release cycle (just after the previous release was cut), we will have more time to verify and still stay relatively up to date with submodules.

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM

@Ivansete-status Ivansete-status changed the title chore: Updating nim-chronicles, nim-chronos, nim-presto, nimcrypto, zerokit, nim-libp2p, and nim-nat-transversal chore: Updating nim-chronicles, nim-chronos, nim-presto, nimcrypto, nim-libp2p, and nim-nat-transversal Sep 18, 2023
@Ivansete-status Ivansete-status merged commit f617cd9 into master Sep 18, 2023
18 checks passed
@Ivansete-status Ivansete-status deleted the update-some-vendor branch September 18, 2023 16:51
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.

5 participants