Skip to content

node2nix: pull in patch to fix bin scripts with crlf line-endings - #216728

Merged
marsam merged 2 commits into
NixOS:masterfrom
lilyinstarlight:fix/node2nix-bin-crlf
Mar 9, 2023
Merged

node2nix: pull in patch to fix bin scripts with crlf line-endings#216728
marsam merged 2 commits into
NixOS:masterfrom
lilyinstarlight:fix/node2nix-bin-crlf

Conversation

@lilyinstarlight

@lilyinstarlight lilyinstarlight commented Feb 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member
Description of changes

Pull in fix from svanderburg/node2nix#314

This is minor fallout from #193337 which made node2nix handle installing script bins itself (since npm stopped doing that correctly)

It seems npm was doing line-ending normalization when installing these files itself, so in addition to making all bins executable we now also do a crlf to lf conversionon on them if they are a script with a shebang

This is rather hacky, but hopefully we'll be able to support most Node.js packages with more modern tooling soon and can use node2nix less in the future

Fixes #216306

cc: @winterqt

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.
nixpkgs-review

Result of nixpkgs-review pr 216728 run on x86_64-linux 1

7 packages marked as broken and skipped:
  • breitbandmessung
  • odoo
  • oni2
  • teleprompter
  • vimPlugins.coc-imselect
  • whalebird
  • zerobin
1 package blacklisted:
  • tests.writers
5 packages failed to build: (note: failures exist on master too)
  • openmoji-black
  • openmoji-color
  • sage
  • sageWithDoc
  • spacegun
207 packages built:
  • airfield
  • akkoma-frontends.akkoma-fe
  • antennas
  • antora
  • balanceofsatoshis
  • base16-builder
  • bat-extras.prettybat
  • bibtex-tidy
  • bitwarden-cli
  • castnow
  • cinnamon.xreader
  • code-server
  • commitlint
  • concurrently
  • create-cycle-app
  • discourse
  • discourseAllPlugins
  • electron-fiddle
  • elmPackages.create-elm-app
  • elmPackages.elm-analyse
  • elmPackages.elm-coverage
  • elmPackages.elm-doc-preview
  • elmPackages.elm-git-install
  • elmPackages.elm-language-server
  • elmPackages.elm-live
  • elmPackages.elm-optimize-level-2
  • elmPackages.elm-pages
  • elmPackages.elm-review
  • elmPackages.elm-test
  • elmPackages.elm-upgrade
  • elmPackages.elm-verify-examples
  • elmPackages.elm-xref
  • emanote
  • emojione
  • epgstation
  • ethercalc
  • fast-cli
  • flexoptix-app
  • flood
  • gnomeExtensions.pop-shell
  • google-clasp
  • grafana-image-renderer
  • gtop
  • haskellPackages.aeson-typescript
  • haskellPackages.emanote
  • haskellPackages.servant-typescript
  • haskellPackages.tailwind
  • hueadm
  • image_optim
  • imapnotify
  • imgbrd-grabber
  • joplin
  • lessc
  • lighthouse
  • mastodon-bot
  • mongosh
  • morgen
  • mx-puppet-discord
  • n8n
  • node2nix
  • onlykey
  • pm2
  • postcss-cli
  • pscid
  • psitransfer
  • pulp
  • purescript-psa
  • pyright
  • python310Packages.batchspawner
  • python310Packages.dockerspawner
  • python310Packages.jupyterhub
  • python310Packages.jupyterhub-ldapauthenticator
  • python310Packages.jupyterhub-systemdspawner
  • python310Packages.jupyterhub-tmpauthenticator
  • python310Packages.oauthenticator
  • python311Packages.batchspawner
  • python311Packages.dockerspawner
  • python311Packages.jupyterhub
  • python311Packages.jupyterhub-ldapauthenticator
  • python311Packages.jupyterhub-systemdspawner
  • python311Packages.jupyterhub-tmpauthenticator
  • python311Packages.oauthenticator
  • quarto
  • redoc-cli
  • reveal-md
  • shepherd
  • shout
  • slack
  • sloc
  • styx
  • tandoor-recipes
  • teams
  • teck-programmer
  • theLoungePlugins.plugins.closepms
  • theLoungePlugins.plugins.giphy
  • theLoungePlugins.plugins.shortcuts
  • theLoungePlugins.themes.abyss
  • theLoungePlugins.themes.amoled
  • theLoungePlugins.themes.amoled-sourcecodepro
  • theLoungePlugins.themes.bdefault
  • theLoungePlugins.themes.bmorning
  • theLoungePlugins.themes.chord
  • theLoungePlugins.themes.classic
  • theLoungePlugins.themes.common
  • theLoungePlugins.themes.crypto
  • theLoungePlugins.themes.discordapp
  • theLoungePlugins.themes.dracula
  • theLoungePlugins.themes.dracula-official
  • theLoungePlugins.themes.flat-blue
  • theLoungePlugins.themes.flat-dark
  • theLoungePlugins.themes.gruvbox
  • theLoungePlugins.themes.hexified
  • theLoungePlugins.themes.ion
  • theLoungePlugins.themes.light
  • theLoungePlugins.themes.midnight
  • theLoungePlugins.themes.mininapse
  • theLoungePlugins.themes.monokai-console
  • theLoungePlugins.themes.mortified
  • theLoungePlugins.themes.neuron-fork
  • theLoungePlugins.themes.new-morning
  • theLoungePlugins.themes.new-morning-compact
  • theLoungePlugins.themes.nologo
  • theLoungePlugins.themes.nord
  • theLoungePlugins.themes.onedark
  • theLoungePlugins.themes.purplenight
  • theLoungePlugins.themes.scoutlink
  • theLoungePlugins.themes.seraphimrp
  • theLoungePlugins.themes.solarized
  • theLoungePlugins.themes.solarized-fork-monospace
  • theLoungePlugins.themes.zenburn
  • theLoungePlugins.themes.zenburn-monospace
  • theLoungePlugins.themes.zenburn-sourcecodepro
  • thelounge
  • triton
  • typescript
  • uppy-companion
  • vimPlugins.YouCompleteMe
  • vimPlugins.coc-clangd
  • vimPlugins.coc-cmake
  • vimPlugins.coc-css
  • vimPlugins.coc-diagnostic
  • vimPlugins.coc-docker
  • vimPlugins.coc-emmet
  • vimPlugins.coc-eslint
  • vimPlugins.coc-explorer
  • vimPlugins.coc-flutter
  • vimPlugins.coc-git
  • vimPlugins.coc-go
  • vimPlugins.coc-haxe
  • vimPlugins.coc-highlight
  • vimPlugins.coc-html
  • vimPlugins.coc-java
  • vimPlugins.coc-jest
  • vimPlugins.coc-json
  • vimPlugins.coc-lists
  • vimPlugins.coc-ltex
  • vimPlugins.coc-markdownlint
  • vimPlugins.coc-metals
  • vimPlugins.coc-nginx
  • vimPlugins.coc-pairs
  • vimPlugins.coc-prettier
  • vimPlugins.coc-pyright
  • vimPlugins.coc-python
  • vimPlugins.coc-r-lsp
  • vimPlugins.coc-rls
  • vimPlugins.coc-rust-analyzer
  • vimPlugins.coc-sh
  • vimPlugins.coc-smartf
  • vimPlugins.coc-snippets
  • vimPlugins.coc-solargraph
  • vimPlugins.coc-spell-checker
  • vimPlugins.coc-sqlfluff
  • vimPlugins.coc-stylelint
  • vimPlugins.coc-sumneko-lua
  • vimPlugins.coc-tabnine
  • vimPlugins.coc-texlab
  • vimPlugins.coc-toml
  • vimPlugins.coc-tslint
  • vimPlugins.coc-tslint-plugin
  • vimPlugins.coc-tsserver
  • vimPlugins.coc-ultisnips
  • vimPlugins.coc-vetur
  • vimPlugins.coc-vimlsp
  • vimPlugins.coc-vimtex
  • vimPlugins.coc-wxml
  • vimPlugins.coc-yaml
  • vimPlugins.coc-yank
  • vimPlugins.markdown-preview-nvim
  • vscode
  • vscode-extensions.matklad.rust-analyzer (vscode-extensions.rust-lang.rust-analyzer)
  • vscode-extensions.ms-python.vscode-pylance
  • vscode-fhs
  • vscode-with-extensions
  • vscodium
  • vscodium-fhs
  • wasm-strip
  • wasm-text-gen
  • wast-refmt
  • webassemblyjs-cli
  • webassemblyjs-repl
  • weylus
  • whitebophir
  • wrangler
  • wring
  • yaml-language-server
  • ycmd
  • zx

@lilyinstarlight lilyinstarlight added 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment backport release-22.11 labels Feb 17, 2023
@lilyinstarlight lilyinstarlight mentioned this pull request Feb 17, 2023
Closed
@ofborg
ofborg Bot requested review from Luflosi, teutat3s and winterqt February 17, 2023 00:44
@ofborg ofborg Bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Feb 17, 2023
@blitz

blitz commented Feb 18, 2023

Copy link
Copy Markdown
Contributor

Works for nodePackages.meshcommander! Thank you!

@blitz

blitz commented Feb 23, 2023

Copy link
Copy Markdown
Contributor

@tfc @RaitoBezarius Can you take a look here or do know the right person to push this over the finishing line? :)

RaitoBezarius
RaitoBezarius approved these changes Feb 25, 2023
View reviewed changes

@RaitoBezarius RaitoBezarius left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Diff looks good, I would wait for @winterqt as she's the Node expert. :-)

@figsoda figsoda added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 5, 2023
@blitz

blitz commented Mar 5, 2023

Copy link
Copy Markdown
Contributor

@winterqt Do you have some time to look into this? :)

@lilyinstarlight

lilyinstarlight commented Mar 5, 2023

Copy link
Copy Markdown
Member Author

I may actually switch this to not use dos2unix, if I can, to avoid having to redo all of the import sites (I originally used it because it had script detection but now that this uses isScript it probably isn't necessary any longer)

See @marsam's review item at svanderburg/node2nix#314 (review)

I'll mark this as draft, and I plan to update this sometime today

@lilyinstarlight
lilyinstarlight marked this pull request as draft March 5, 2023 15:38
@lilyinstarlight
lilyinstarlight force-pushed the fix/node2nix-bin-crlf branch 2 times, most recently from df97fbc to 06ed810 Compare March 6, 2023 14:14
@lilyinstarlight
lilyinstarlight marked this pull request as ready for review March 6, 2023 14:15
@blitz

blitz commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

I've checked the updated PR and it works. 👍

@lilyinstarlight
node2nix: pull in patch to fix bin scripts with crlf line-endings
b85c1e3 
This is minor fallout from svanderburg/node2nix#302 which made node2nix
handle installing script bins itself (since npm stopped doing that
correctly).

It seems npm was doing line-ending normalization when installing these
files itself, so in addition to making all bins executable we now also
do a crlf to lf conversion on them if they are a script with a shebang.
@lilyinstarlight
lilyinstarlight force-pushed the fix/node2nix-bin-crlf branch from 06ed810 to 4df1c39 Compare March 7, 2023 15:22
@lilyinstarlight
lilyinstarlight force-pushed the fix/node2nix-bin-crlf branch from 4df1c39 to 0835e5a Compare March 7, 2023 15:23
marsam
marsam approved these changes Mar 8, 2023
View reviewed changes

@marsam marsam left a comment

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.

LGTM. Thank you!

@marsam
marsam merged commit 2e80f1e into NixOS:master Mar 9, 2023
@github-actions github-actions Bot mentioned this pull request Mar 9, 2023
Merged
1 task
@github-actions

github-actions Bot commented Mar 9, 2023

Copy link
Copy Markdown
Contributor

Successfully created backport PR for release-22.11:

@lilyinstarlight
lilyinstarlight deleted the fix/node2nix-bin-crlf branch March 9, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RaitoBezarius RaitoBezarius RaitoBezarius approved these changes
@teutat3s teutat3s Awaiting requested review from teutat3s
@Luflosi Luflosi Awaiting requested review from Luflosi
@winterqt winterqt Awaiting requested review from winterqt
+1 more reviewer
@marsam marsam marsam approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

nodePackages binaries fail because of DOS line endings

5 participants

@lilyinstarlight @blitz @marsam @RaitoBezarius @figsoda