Skip to content

enhance README and Makefile#3861

Draft
Ivansete-status wants to merge 11 commits into
masterfrom
update-readme
Draft

enhance README and Makefile#3861
Ivansete-status wants to merge 11 commits into
masterfrom
update-readme

Conversation

@Ivansete-status
Copy link
Copy Markdown
Collaborator

No description provided.

Co-authored-by: Copilot <copilot@github.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3861

Built from 20b5d50

@Ivansete-status Ivansete-status marked this pull request as draft May 7, 2026 17:18
Copy link
Copy Markdown
Contributor

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment thread README.md Outdated
Comment on lines +29 to +33
Supported toolchain versions:
- Nim 2.2.4
- Nimble 0.22.3

These versions are enforced by the repository configuration and the Makefile.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see a conflict between "supported" and "enforced by the repository"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Rephrased in b7a549b

Comment thread README.md Outdated
The easiest way to install these, is using `rustup`:

Supported toolchain versions:
- Nim 2.2.4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't Nim installed by Nimble? So only Nimble is required, no?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes but afaik, a nimble version implies a certain Nim version. Then, if we want to use nimble 0.22.3, that would install nim 2.2.8 but we don't want/need to bump the compiler yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But don't we specify Nim version in the nimble file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In the nimble file we specify a minimum supported version. We suggest the use of 2.2.4 Nim compiler but it may also compile with larger versions.
The README.md now suggests:

Recommended and tested toolchain versions:
...

The script looks correct now. Here's the summary of what changed and why:

scripts/install_nim.sh — two changes:

Moved NIM_DEST definition to the top (line 20), before the early-exit check that references it.

Replaced the early-exit condition. The old logic checked nim --version in PATH and exited if it matched — including a silent exit for "newer version already installed". The problem: on the Linux machine, nim 2.2.4 was installed by the system package manager at a non-standard path (e.g. /usr/bin/nim or /nix/store/…). The script saw the right version and exited early, but ~/.nim/nim-2.2.4/ was never created. Later, when nimble fell back to downloading nimble 0.22.3 source and compiling it, nim looked for its stdlib at ~/.nim/nim-2.2.4/lib/system.nim — which didn't exist.

The new logic: only skip if ${NIM_DEST}/lib/system.nim actually exists (i.e. we installed it there). If it does, re-create the ~/.nimble/bin/ symlinks just in case they're stale, then exit. Otherwise, always download and install to ~/.nim/nim-2.2.4/, creating a self-contained installation the rest of the build can rely on.
@stubbsta stubbsta self-assigned this May 19, 2026
@stubbsta
Copy link
Copy Markdown
Contributor

stubbsta commented May 19, 2026

From discussions on discord https://discord.com/channels/973324189794697286/1504518624818368512:

Running make update pulls nimble v2.2.10 and updates the nimble.lock file but the recommended version is v2.2.4 and we want the lock file to remain unchanged.

We need to verify whether the actions performed by make update is indeed needed - it looks like make wakunode2 already performs al the required actions.

@stubbsta
Copy link
Copy Markdown
Contributor

@Ivansete-status please also have a look at my changes, I can't add you as a reviewer because this PR is assigned to you

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