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/update nim version #1052

Merged
merged 47 commits into from
Jan 10, 2025
Merged

Chore/update nim version #1052

merged 47 commits into from
Jan 10, 2025

Conversation

2-towns
Copy link
Contributor

@2-towns 2-towns commented Dec 18, 2024

Update Nim to 2.0.12 2.0.14

@@ -90,7 +90,7 @@ proc init*(_: type RestNode, node: dn.Node): RestNode =
peerId: node.record.data.peerId,
record: node.record,
address: node.address,
seen: node.seen
seen: node.alreadySeen()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cskiraly Is this the right method to call to verify that a node is seen?

Copy link
Contributor

Choose a reason for hiding this comment

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

alreadySeen tells you whether it had been already seen (responded) at least once.

seen, instead, gives you a exponential moving average of responses (or missing responses), with more weight on recent attempts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay so if I want to detect the connection with a peer is "good enough", I could check that node.seen > 0.5 I guess ? 0.5 is totally arbitrary.

if: inputs.os == 'windows'
shell: ${{ inputs.shell }} {0}
run: |
git config --global core.symlinks false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@veaceslavdoina Double check if it is okay for you

@2-towns 2-towns marked this pull request as ready for review January 8, 2025 10:46
expect Defect:
await raiseStreamException(newException(CatchableError, "test error"))
# This test cannot exist anymore.
# The signature of the method readOnce is explicitly listing the error raised:
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I think we can get rid of this test completely.

@dryajov
Copy link
Contributor

dryajov commented Jan 9, 2025

Great job! This is looking really good, lets try to merge this ASAP

if: inputs.os == 'windows'
shell: ${{ inputs.shell }} {0}
run: |
pacman -U --noconfirm https://repo.msys2.org/mingw/ucrt64/mingw-w64-ucrt-x86_64-gcc-13.2.0-6-any.pkg.tar.zst https://repo.msys2.org/mingw/ucrt64/mingw-w64-ucrt-x86_64-gcc-libs-13.2.0-6-any.pkg.tar.zst
pacman -U --noconfirm https://repo.msys2.org/mingw/ucrt64/mingw-w64-ucrt-x86_64-gcc-14.2.0-2-any.pkg.tar.zst https://repo.msys2.org/mingw/ucrt64/mingw-w64-ucrt-x86_64-gcc-libs-14.2.0-2-any.pkg.tar.zst
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows is installing latest GCC version, which currently is 14 and 15 is on the way :)

We can remove this step, but later we might have an issue with version 15 (?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Windows 2025 right ?

But the windows-latest is still pointing on windows-2022 which is using gcc 12.

Copy link
Contributor

@veaceslavdoina veaceslavdoina Jan 9, 2025

Choose a reason for hiding this comment

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

We are not relying on the preinstalled GCC on Windows. It is about MSYS2 and how it works

MSYS2 is a rolling release distribution

At some point, it will just install latest GCC and version 15 will be released in March~April (#875 (comment)). It will appear in MSYS2 repository with a delay and we can expect an issue in August, as we had it with the version 14 😄

So, if we remove that step, we can get in the the issue in August, but will have latest GCC releases all the time.

Let's leave it to have a stable CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not relying on the preinstalled GCC on Windows. It is about MSYS2 and how it works

Ah ok.

Let's leave it to have a stable CI?

Yeah I agree. I prefer to leave it for now and after testing GCC 15, we could remove it.

@@ -40,6 +40,7 @@ jobs:
os: ${{ matrix.os }}
shell: ${{ matrix.shell }}
nim_version: ${{ matrix.nim_version }}
coverage: false
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add an input coverage to the .github/actions/nimbus-build-system/action.yml and set it false by default, than pass true just for coverage (we are doing it already).

https://github.com/codex-storage/nim-codex/actions/runs/12689785802
Screenshot 2025-01-09 at 16 12 51

os {linux}, cpu {amd64}, builder {ubuntu-20.04}, tests {contract}, nim_version {${{ env.nim_version }}}, shell {bash --noprofile --norc -e -o pipefail}
os {linux}, cpu {amd64}, builder {ubuntu-20.04}, tests {integration}, nim_version {${{ env.nim_version }}}, shell {bash --noprofile --norc -e -o pipefail}
os {linux}, cpu {amd64}, builder {ubuntu-20.04}, tests {tools}, nim_version {${{ env.nim_version }}}, shell {bash --noprofile --norc -e -o pipefail}
os {linux}, cpu {amd64}, builder {ubuntu-latest}, tests {unittest}, nim_version {${{ env.nim_version }}}, shell {bash --noprofile --norc -e -o pipefail}
Copy link
Contributor

Choose a reason for hiding this comment

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

We switched to ubuntu-20.04 in #939 and because of the #932.

Probably we should stick with the 20.04?

Btw, we also have a release workflow and 20.04 is required mostly for releases. But as I remember, there can be an issue when cache from latest (24.04) is restored on 20.04 and vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably we should stick with the 20.04?

There are two problems: 1- 20.04 will be deprecated soon, 2- I am not sure I will be able to install GCC 14 on ubuntu 20.04.

Maybe #932 is fixed now with the update to Nim 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was related to the glibc version. So, new releases will not run on old distributions.

The Ubuntu 20.04 Actions runner image will begin deprecation on 2025-02-01 and will be fully unsupported by 2025-04-01 #11101

Deprecation will begin on 2025-02-01 and the image will be fully unsupported by 2025-04-01.

The Ubuntu lifecycle and release cadence
Screenshot 2025-01-09 at 17 05 20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is what we discussed yesterday.

@benbierens
Copy link
Contributor

benbierens commented Jan 9, 2025

Running make clean, make update, and make, works without any issues on my Win11 MSYS2-UCRT.
Commit 801f6b7 - Disable CI cache for coverage.
Make test also builds, and passes.

@2-towns 2-towns added this pull request to the merge queue Jan 10, 2025
Merged via the queue into master with commit f25c555 Jan 10, 2025
17 checks passed
@2-towns 2-towns deleted the chore/update-nim-version branch January 10, 2025 15:29
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