Skip to content

Conversation

@narimiran
Copy link
Contributor

No description provided.

@arnetheduck
Copy link
Member

well, it can - it should pick an earlier chronicles version - what's needed is to update nimble

@narimiran
Copy link
Contributor Author

it should pick an earlier chronicles version

Even the latest nimble wouldn't help with that as the testing command specifically wants the latest chronicles:

nimble install -y chronicles@#head

@arnetheduck
Copy link
Member

specifically wants

that's likely from before we started using tagged versions in chronicles and when nimble was completely broken - ie tagged releases of chronices were made only recently - we can remove #head now as well because this is the experience we want users to have - with the work invested in nimble, things should "just work" and removing #head and other ugly hacks is a step in that direction.

@arnetheduck
Copy link
Member

getting closer - now all that's needed is a nimble override (using github.com/status-im/nimbus-common-workflow/pull/6) - alternatively, we should simply switch nimbus-build-system to always build our version of nimble.

@narimiran
Copy link
Contributor Author

getting closer

And with the latest commit, using the latest nimble: getting further away :'(

@arnetheduck
Copy link
Member

ah interesting, looks like it doesn't pollute the compilation any more with system-installed packages, in the new mode cc @jmgomez is this intentional?

--requires:chronicles should allow running a single command with chronicles installed

@arnetheduck
Copy link
Member

arnetheduck commented Sep 22, 2025

in the meantime, I think all relevant bugs were fixed by the time of the 0.20.1 release (which is where we fixed the version filtering I think)

@jmgomez
Copy link

jmgomez commented Sep 23, 2025

nimble v0.20.1 compiled at 2025-09-22 18:45:16

As mentioned in the discord channel, the fallback is only implemented in 0.99.0. At this point should be more correct that 0.20.0 already. But of course there may be bugs. Some other Status libraries are using 0.99.0 with no issues. IMO is a good stress test to start using it gradually (with a fix commit for now til 1.0.0) as we could quickly iterate to fix any potential issue

@jmgomez
Copy link

jmgomez commented Sep 23, 2025

@narimiran if you dont want to update to 0.99.0 another thing that could be attempted is to use --require:nim==NimVersion where NimVersion is the system nim you are testing against. But I dont know if this will work as sometimes it fallbacks into the greedy solver. IMO it would be much better regardless the case to start trying out 0.99.0

@narimiran
Copy link
Contributor Author

using 0.99.0 with no issues

That version is not in the tags/releases: https://github.com/nim-lang/nimble/tags. But I found it in the commit list: nim-lang/nimble@db52507 - let me try to use that one and see what breaks :)

@jmgomez
Copy link

jmgomez commented Sep 23, 2025

Yeah, its because we are not releasing it yet. It will become 1.0.0 at some point in the future, once it is complete and there is enough testing over it.

Better to use the latest commit though :)

@narimiran
Copy link
Contributor Author

Both the latest commit and 0.99.0 are failing on all 1.6 runners. The 0.20.1 is failing only on Windows, other 1.6 runners are ok.

@jmgomez
Copy link

jmgomez commented Sep 23, 2025

hmm ok. Then I would recommend to stick to 0.20.1 and disable the Win test temporary if thats an option. I will make this a priority as soon as I come back to the time off Ill be taken

@arnetheduck
Copy link
Member

The 0.20.1 is failing only on Windows, other 1.6 runners are ok.

uh .. this doesn't make sense at all - why would windows be special in this regard?

@narimiran
Copy link
Contributor Author

uh .. this doesn't make sense at all - why would windows be special in this regard?

  • using nimble v0.20.1 with nimble --requires:unittest2 --requires:chronicles test_chronicles, produces on Windows:

    C:\Users\runneradmin\.nimble\pkgs2\chronicles-0.12.2-02febb20d088120b2836d3306cfa21f434f88f65\chronicles\options.nim(157, 32) Error: expression 'NoColors' is of type 'ColorScheme' and has to be used (or discarded)
    

    which was the error on all runners with nimble v0.20.1 in the previous commit, when the nimble command didn't use explicit --requires:....

  • using nimble v0.99.0 with the same command as above, produces on all runners:

    /home/runner/work/nim-metrics/nim-metrics/tests/main_tests.nim(7, 17) Error: cannot open file: unittest2
    

    This is a regression from v0.20.1 to v0.99.0.

@arnetheduck
Copy link
Member

using nimble v0.20.1 with nimble --requires:unittest2 --requires:chronicles test_chronicles, produces on Windows:

yeah, I saw - if we look at the logs, it goes on to install nim 2.2.4 which it shouldn't do, which is effectively the bug/feature in nimble.

there's another way we can work around this problem for now, in metrics.nimble:

when (NimMajor, NimMinor) < (2, 0):
  taskRequires "test_chronicles", "chronicles < 0.12"

If we do that hack, it should be a high nimble prio to make it "just work" without the explicit instruction (this approach obviously doesn't scale).

@jmgomez
Copy link

jmgomez commented Oct 13, 2025

Why the test task has skipParentCfg? When removing it, it passes locally when system nim is 1.6.20 with latest nimble commit (only tested in win so far)

@tersec
Copy link
Contributor

tersec commented Oct 13, 2025

Why the test task has skipParentCfg? When removing it, it passes locally when system nim is 1.6.20 with latest nimble commit (only tested in win so far)

This enables it, for example to be more effectively used within the nimbus-eth1/nimbus-eth2 spaces. See:

For example for what went wrong when nimble attempted to require not using skipParentCfg. It was a vexing time, would prefer not to repeat it.

@jmgomez
Copy link

jmgomez commented Oct 14, 2025

@tersec I see. Im failing to see the fundamental change between nim 1.6.x and 2.x. It seams at some point they treated the flag skipParentCfg differently, ie 1.6.x ignores the one in the project which makes this test to fall apart as it relies in the config.nims to retrieve the paths indirectly (via using nim c).

If Im not missing another way to specify the paths to the old compiler (which may be the case, I saw some references to __NIMBLE_PATHS but in my local tests it didnt produce any effect), either this project needs to remove the flag when nim is 1.6.x or the test executions needs to be reformulated

@jmgomez
Copy link

jmgomez commented Oct 14, 2025

or the test executions needs to be reformulated

this also includes using the global flag btw. In any case, doesnt seem to be a nimble issue

@jmgomez
Copy link

jmgomez commented Oct 15, 2025

@narimiran notice the issue is in the project. Old nimble by default is global hence 1.6 can find the path to the deps as they are globally available even when the config file is ignored.

@arnetheduck
Copy link
Member

arnetheduck commented Oct 17, 2025

Old nimble by default is global hence 1.6 can find the path to the deps as they are globally available even when the config file is ignored.

this is, I guess, the root cause of some of the issues here - afair, no nim versions look for dependencies in local nimbledeps which is why install and the like will not work, while nimble setup should ..

in fact, looking at the way the project is built, there's no way for nim c ... to work in local mode because nim will not know where to look for dependencies. this is something to consider, ie without creating a paths file, tasks cannot work -> in local mode, nimble "must" implicitly run setup for things to work.

this is also something to keep in mind for taskRequires - it needs to update the paths file while the task is running and then restore at the end of the task

@narimiran
Copy link
Contributor Author

narimiran commented Oct 27, 2025

From my point of view, this PR can be (cleaned up and) merged:

  • it uses a newer Nimble than what is shipped with Nim 1.6
  • this correctly picks up an older version of chronicles which still supports Nim 1.6
  • this allows metrics to run tests on Nim 1.6
  • the tests are green

Why it works for this Nimble commit and not for the newest one is an orthogonal Nimble problem, and shouldn't block merging of this PR.

@narimiran narimiran merged commit 9b9afee into master Nov 3, 2025
27 checks passed
@narimiran narimiran deleted the remove-16 branch November 3, 2025 08:15
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.

4 participants