Upgrade to Nim 2.2.10, bump ecosystem deps, add nimpretty formatter - #412
Upgrade to Nim 2.2.10, bump ecosystem deps, add nimpretty formatter#412karolyi wants to merge 1 commit into
Conversation
Nim 2.2.x and Dependency UpgradesThis document details all changes made to support Nim 2.2.10 and the newest Nim Version
Dependency Versions
New transitive dependencies added to Source Code Changes
|
|
I've spent a whole evening with this after getting frustrated that the langserver wasn't really functioning as intended. As it turned out, this was the crucial fix: + # LSP allows null or absent params; json_rpc 0.6+ decoder requires array/object
+ if contentJson.getOrDefault("params").kind == JNull:
+ contentJson["params"] = newJObject()
+ # Notifications have no id; RequestRx.id is non-optional so inject null
+ # to avoid raiseIncompleteObject during decode.
+ if "id" notin contentJson:
+ contentJson["id"] = newJNull()This made the entire langserver smooth again and my sublime text also doesn't go crazy anymore. |
|
Thanks for your work. However before we can merge this a few changes has to be made:
|
|
Before I dissect the changes to separate commits, I need a test run which I can't start here. |
|
Need another test run but good to go afaik. The "ubuntu hash mismatch" error is nothing I can make sense of. I'm on Artix linux (on this machine), and nimble wrote these hashes. |
|
I've tried to satisfy the ubuntu test env and modified the checksum to be the one it expects, but now I have another error. People on IRC told me it's more of a ubuntu related problem now, so I'm not sure how to proceed. Suggestions are welcome. |
Nim / dependency versions:
- Nim: 2.0.8 → 2.2.10; chronos: 4.0.4 → 4.2.2; json_rpc: 0.5.0 → 0.6.0
- chronicles: 0.10.3 → 0.12.2; serialization: 0.2.6 → 0.5.2
- json_serialization: 0.2.9 → 0.4.4; stew: 0.2.0 → 0.5.0
- faststreams: 0.3.0 → 0.5.1; websock: 0.2.0 → 0.3.0; plus patch bumps for
bearssl, nimcrypto, stint, zlib, httputils, testutils, unittest2, regex
Compatibility fixes for new library APIs:
- json_rpc 0.6: req.meth → req.method.get(""); decoder takes string not
JsonNode; inject null id for LSP notifications, null→{} for null params
- chronicles 0.12: chronicles_disable_thread_id → chronicles_thread_ids=no;
JsonNode log args wrapped in $() throughout all call sites
- Nim 2.2 gcsafe: add {.gcsafe.} to addCallback closures in ls.nim
- nimble dump: pass workingDir instead of file path as CLI argument
- lspsocketclient: manual requestId counter, rename address→connAddress,
fix error-cleanup loop to use responses table, tighten proc pragmas
Formatter improvements (routes/lsp.nim, ls.nim):
- Add NlsFormatter enum and nim.formatter config option (nph | nimpretty)
- Add getNimprettyPath/getFormatterPath helpers; advertise
documentFormattingProvider when either formatter binary is on PATH
- Fix pipe-full deadlock in format proc: drain stderr concurrently before
waiting for process exit; add defer closeWait for process cleanup
- Fix CancelledError path: kill + reap zombie process before returning
- Fix typos in log strings (doenst, formating, modifyed)
Add test_hover.py: standalone Python LSP smoke-test script
|
Bump. I still don't know what's wrong with ubuntu. |
| stderrHandle = AsyncProcess.Pipe, | ||
| ) | ||
| let res = await process.waitForExit(InfiniteDuration) | ||
| defer: await noCancel(process.closeWait()) |
There was a problem hiding this comment.
closeWait doesn't require noCancel: https://status-im.github.io/nim-chronos/concepts.html#nocancel
|
@karolyi Hi! Sorry for the slow response. What is exactly failing with Ubuntu? I'm not seeing the CI run. UPD: found it: https://github.com/nim-lang/langserver/actions/runs/26177730410/job/77729875902 UPD 2: Apparently, you're facing another error. Please send it. I'm testing locally on Ubuntu and not seeing any errors.
These things need to be done as well. I think if you split the PR into three (Nim 2.2.10 update, deps update, and adding formatter) it will be much easier to isolate the issue with the CI. |
| "downloadMethod": "git", | ||
| "dependencies": [], | ||
| "checksums": { | ||
| "sha1": "19c7dc767547c8c8252f4f26f5853ca574f41eb7" |
There was a problem hiding this comment.
@karolyi this is the wrong checksum though, this is why the build won't pass.
Run nimble install -ldy && nimble lock to update the lock file with the correct checksum (or simply edit it manually to 17ec440fdb89f8903db29a17898af590087d2b64).
There was a problem hiding this comment.
Nimble 0.20.1 used in CI would install the version with sha1 19c7.., newer Nimble would install 17ec44...
This is why the CI failed. I've updated Nimble version, the tests pass now: #432
| | `nim.formatOnSave` | Format on save (requires `nph` or `nimpretty` on `PATH`). | | ||
| | `nim.formatOnSave` | Format the file on save. Requires `nph` or `nimpretty` to be available in the PATH. | |
| # waitpid, leaving the process as a zombie. Kill and reap explicitly. | ||
| discard process.kill() | ||
| discard await noCancel(stderrFuture) | ||
| try: discard await noCancel(process.waitForExit(InfiniteDuration)) |
There was a problem hiding this comment.
Weird try-except fomatting, hard to read.
There was a problem hiding this comment.
@karolyi do we need this file at all though? Hover is already coverer in tnimlangserver.nim. It's not run as part of the test suite too.
There was a problem hiding this comment.
I needed this to test locally with something independent from nim itself. I'd leave it there just for that, but it's your call. I'm mostly a pythonista so I needed to test the actual results in something that is proven to work.
There was a problem hiding this comment.
Please remove it from the PR. I totally get the you needed it for local testing but since it's not part of the test suite and duplicates an existing test, it can be safely removed.
There was a problem hiding this comment.
@karolyi please rename it back to testhelpers.nim. This will shrink the PR size.
|
I've cherrypicked the changes that update the deps into a separate PR: #432 |
|
Feel free to cherrypick this PR to its parts, but I'd like to get some credits on it since I've already put in the work. :) |
|
@karolyi absolutely! It is your PR, I'm just splitting it into pieces to help it get merged faster. |
Nim / dependency versions:
Compatibility fixes for new library APIs:
Formatter improvements (routes/lsp.nim, ls.nim):
Add test_hover.py: standalone Python LSP smoke-test script