Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: replace NodeJS with NodeJS_18_jll #1069

Closed
wants to merge 1 commit into from
Closed

refactor: replace NodeJS with NodeJS_18_jll #1069

wants to merge 1 commit into from

Conversation

stephen-huan
Copy link

This PR replaces NodeJS.jl with the JLL package NodeJS_18_jll.jl built from Yggdrasil.

This also indirectly fixes an issue with NodeJS.jl not declaring cxxstring_abi.

Note 1. I tested the PR on my personal website (source) and it prerendered pages with both KaTeX and highlight.js.

Note 2. The diff on Project.toml is misleadingly large since it re-ordered some entries.

In order to update Project.toml, I performed the following REPL commands.

]remove NodeJS
]add NodeJS_18_jll

Note 3. There is a slightly higher maintenance burden as the jll will have to be manually updated to new major versions.

Note 4. This PR isn't critical to getting my website to build since there's the $NODE environmental variable escape hatch that I discovered while implementing this PR. Perhaps there should be a little note in the documentation or the README about this, but then again, missing the C++ standard library on Linux is pretty niche.

Note 5. I implemented the PR with the following substitutions

Note that a strict, literal replacement would look like

  • nodejs_cmd() -> `$(node().exec[1])`
  • npm_cmd() -> `$(node().exec[1]) $npm`

However, in the context of a run(...) it's possible to do

  • run(`$(nodejs_cmd()) $arguments`) -> run(`$(node()) $arguments`)
  • run(`$(npm_cmd()) $arguments`) -> run(`$(node()) $npm $arguments`)

motivating the simpler implementation.

Note that the difference is nontrivial, as the BinaryBuilder.jl documentation explains:

A common point of confusion about ExecutableProducts in JLL packages
is why these function wrappers are needed: while in principle you could
run the executable directly by using its absolute path in run, these
functions ensure that the executable will find all shared libraries it
needs while running.

Indeed, using the literal substitutions breaks when it can't find the shared libraries,

julia> using NodeJS_18_jll

julia> run(`$(node().exec[1]) -v`)
/nix/store/kyzad6jxd5dnnf88xvx5m19fgqccqs56-NodeJS_18/bin/node: error while loading shared libraries: libstdc++.so.6: cannot open shared object file: No such file or directory
ERROR: failed process: Process(`/nix/store/kyzad6jxd5dnnf88xvx5m19fgqccqs56-NodeJS_18/bin/node -v`, ProcessExited(127)) [127]

Stacktrace:
 [1] pipeline_error
   @ ./process.jl:565 [inlined]
 [2] run(::Cmd; wait::Bool)
   @ Base ./process.jl:480
 [3] run(::Cmd)
   @ Base ./process.jl:477
 [4] top-level scope
   @ REPL[3]:1

julia> run(`$(node()) -v`)
v18.16.1
Process(setenv(`/nix/store/kyzad6jxd5dnnf88xvx5m19fgqccqs56-NodeJS_18/bin/node -v`,[..., "CXX=g++", ..., "LD_LIBRARY_PATH=/nix/store/948q7s8iyrdccpqvi6z785njabcjd204-julia-1.9.4/bin/../lib/julia:/nix/store/948q7s8iyrdccpqvi6z785njabcjd204-julia-1.9.4/bin/../lib", ...]), ProcessExited(0))

which is also why $npm cannot be used directly, as it needs to be in node()'s environment.

julia> run(`$npm -v`)
ERROR: IOError: could not spawn `/nix/store/kyzad6jxd5dnnf88xvx5m19fgqccqs56-NodeJS_18/bin/npm -v`: no such file or directory (ENOENT)
Stacktrace:
 [1] _spawn_primitive(file::String, cmd::Cmd, stdio::Vector{Union{RawFD, IO}})
   @ Base ./process.jl:128
 [2] #760
   @ ./process.jl:139 [inlined]
 [3] setup_stdios(f::Base.var"#760#761"{Cmd}, stdios::Vector{Union{RawFD, IO}})
   @ Base ./process.jl:223
 [4] _spawn
   @ ./process.jl:138 [inlined]
 [5] run(::Cmd; wait::Bool)
   @ Base ./process.jl:479
 [6] run(::Cmd)
   @ Base ./process.jl:477
 [7] top-level scope
   @ REPL[2]:1

julia> run(`$(node()) $npm -v`)
9.5.1
Process(setenv(`/nix/store/kyzad6jxd5dnnf88xvx5m19fgqccqs56-NodeJS_18/bin/node /nix/store/kyzad6jxd5dnnf88xvx5m19fgqccqs56-NodeJS_18/bin/npm -v`,[...]), ProcessExited(0))

Here are examples of all the listed substitutions.

julia> using NodeJS

julia> using NodeJS_18_jll

julia> nodejs_cmd()
`/home/ikue/.julia/artifacts/9c278c61d6242d19deca58e582fc6a6f0a727de8/bin/node`

julia> `$(node().exec[1])`
`/home/ikue/.julia/artifacts/2b2da0a2ed252455e18961d79ed6875839493988/bin/node`

julia> run(`$(nodejs_cmd()) -v`)
v18.16.0
Process(`/home/ikue/.julia/artifacts/9c278c61d6242d19deca58e582fc6a6f0a727de8/bin/node -v`, ProcessExited(0))

julia> run(`$(node()) -v`)
v18.16.1
Process(setenv(`/home/ikue/.julia/artifacts/2b2da0a2ed252455e18961d79ed6875839493988/bin/node -v`, [...]), ProcessExited(0))

julia> npm_cmd()
`/home/ikue/.julia/artifacts/9c278c61d6242d19deca58e582fc6a6f0a727de8/bin/node /home/ikue/.julia/artifacts/9c278c61d6242d19deca58e582fc6a6f0a727de8/bin/npm`

julia> `$(node().exec[1]) $npm`
`/home/ikue/.julia/artifacts/2b2da0a2ed252455e18961d79ed6875839493988/bin/node /home/ikue/.julia/artifacts/2b2da0a2ed252455e18961d79ed6875839493988/bin/npm`

Note 6. The run(`$(node()) $arguments`) syntax requires Julia v1.6+.

The former form is only available when using Julia v1.6, but should be
preferred going forward, as it is thread-safe and generally more flexible.

If compatibility with old Julia versions is important, the legacy syntax

node() do exe
    run(`$exe $arguments`)
end

can be used instead.

@stephen-huan
Copy link
Author

I forgot to mention this, but I did run the tests (test.txt) and they pass locally. I didn't think any of them tested the prerendering functionality, however (note that node is missing in the test log and the tests still passed).

(node is not able to be executed due to a quirk of my operating system, not an issue with the PR)

@tlienart
Copy link
Owner

Hey, I'm sorry I never reacted to your PR, thanks for opening it and giving plenty of details + suggestions. With https://github.com/davidanthoff/NodeJS.jl/releases/tag/v2.0.0 which uses N18, I think for now I'll keep things as they are as I'm a bit stretched on the maintenance front and leaning on NodeJS.jl makes my life a bit easier. I may revisit this in the future though and I'm grateful you explored it in details, it also looks like not merging this won't affect you.

Thanks & I hope this makes sense!

@tlienart tlienart closed this Apr 16, 2024
@stephen-huan
Copy link
Author

No worries! For anyone in the same position as me, note 4 gives an easy workaround anyways.

configurePhase = ''
  runHook preConfigure


  export LOCALE_ARCHIVE="${glibcLocales}/lib/locale/locale-archive"
  export LANG=en_US.UTF-8
  # give a valid node binary to Franklin.jl
  # https://github.com/tlienart/Franklin.jl/pull/1069
  NODE="$(which node)"
  export NODE


  runHook postConfigure
'';

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.

2 participants