sieve-editor-gui: Pin nodejs to nodejs22#477694
Conversation
|
@Silver-Golden could you have a look? |
nixpkgs-ci
Bot
added
10.rebuild-linux: 1-10
Silver-Golden
left a comment
Silver-Golden
left a comment
There was a problem hiding this comment.
Thank ye so much for doing this, I have an open PR upstream (thsmi/sieve#1128) which should fix it.
Had been meaning to patch it here but got too busy.
nixpkgs-ci
Bot
added
12.approvals: 1
|
@NixOS/nixpkgs-merge-bot merge |
|
@Silver-Golden wants to merge this PR. Requirements to merge this PR with
❌ Pull Request could not be merged (#305350) |
|
Danm, had hoped that would work |
| in | ||
|
|
||
| buildNpmPackage { | ||
| buildNpmPackage.override { nodejs = nodejs_22; } { |
There was a problem hiding this comment.
| buildNpmPackage.override { nodejs = nodejs_22; } { | |
| buildNpmPackage { |
Doesn't need to be an override, just provide it as normal.
There was a problem hiding this comment.
@khaneliman the override is absolutely required right now as recently the default node changed from v22 to v24 (#462407)
This package is failing because it is not playing nicely with node v24.
There is a patch for upstream in progress as mentioned earlier (#477694 (review))
This patch by lzszt is intended to mitigate the impact here in nixpkgs while we wait for it to get fixed upstream.
So unless ye have permission to actually merge in the PR would you kindly remove this review.
There was a problem hiding this comment.
@khaneliman the override is absolutely required right now as recently the default node changed from v22 to v24 (#462407)
This package is failing because it is not playing nicely with node v24.
There is a patch for upstream in progress as mentioned earlier (#477694 (review))
This patch bylzsztis intended to mitigate the impact here in nixpkgs while we wait for it to get fixed upstream.
You completely misunderstand my review... I've merged a couple of these. It doesn't need this syntax. You can provide it as nodejs = nodejs_22 below... example of one https://github.com/NixOS/nixpkgs/pull/477638/changes
So unless ye have permission to actually merge in the PR would you kindly remove this review.
Not exactly an appropriate response, regardless of a person's privileges involved in a review.
There was a problem hiding this comment.
The link to the other PR changes things because it is explicit.
In the diff for the review you basically undid the change that this PR is making while not supplying the alternative.
Which kinda makes it look as if ye were calling the PR pointless.
So with teh information provided there was no misunderstanding from my point of view.
Regardless of that I am not too keen of nodejs = nodejs_22; in this case, its too neat/tidy.
.override { nodejs = nodejs_22; } is uglier and far more likely to get me to come back and ensure the root cause gets fixed properly.
Thats personal preference though, in reality either work to get the package being built again.
There was a problem hiding this comment.
In the diff for the review you basically undid the change that this PR is making while not supplying the alternative.
Which kinda makes it look as if ye were calling the PR pointless.
So with teh information provided there was no misunderstanding from my point of view.
"Doesn't need to be an override, just provide it as normal."
Regardless of that I am not too keen of nodejs = nodejs_22; in this case, its too neat/tidy.
.override { nodejs = nodejs_22; } is uglier and far more likely to get me to come back and ensure the root cause gets fixed properly.
Thats personal preference though, in reality either work to get the package being built again.
Scroll through the list of https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+pin+nodejs+is%3Aclosed and try to follow the community standard. Not sure why PR feedback is being argued here for a single 1 line change suggestion from someone.
There was a problem hiding this comment.
+1 for doing things via the intended interface. Adding a comment that it should be revisited might be a good idea?
There was a problem hiding this comment.
I've changed the PR to only set nodejs.
lzszt
force-pushed
the
sieve-editor-gui-node_js
branch
from
6114be6 to
1b339f2
Compare
| pname = "sieve-editor-gui"; | ||
| version = "0.6.1-unstable-2025-03-12"; | ||
|
|
||
| nodejs = nodejs_22; |
There was a problem hiding this comment.
As mentioned above, adding a comment would be nice.
This is especially good if you are concerned about having a workaround in the package for long term. When it's fixed, we can find redundand workarounds and remove these nodejs pins.
| nodejs = nodejs_22; | |
| # npm dependency install fails with nodejs_24: https://github.com/NixOS/nixpkgs/issues/474535 | |
| nodejs = nodejs_22; |
5 participants
Fixes the build failure of
sieve-editor-guifrom #477693, by pinningnodejsto version 22.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.