Repository navigation
Conversation
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
Note that node support for this platform is experimental, which isn't a category one should expect tools to automatically support.
That said, this change would need thorough tests.
|
I just ran a |
Is there anything you need me to help with for this task? |
|
@hqzing yes, i'd need you to add tests. |
|
@ljharb I've added stub-based tests for OpenHarmony detection. Happy to scope the coverage differently if you'd prefer. Added
The test covers three scenarios:
What I deliberately did not add tests for, and whyI audited every function in the install pipeline that reads
|
| # OpenHarmony cannot be reliably detected via `uname` because the device | ||
| # may run either a Linux kernel or a HarmonyOS kernel; detect it from the | ||
| # userland instead. | ||
| if command param get const.ohos.fullname 2>/dev/null | nvm_grep -q "OpenHarmony"; then | ||
| NVM_OS=openharmony | ||
| fi |
There was a problem hiding this comment.
This probe is ungated: it runs after the case on every platform and unconditionally overwrites whatever uname already determined. Two consequences.
1. It can override a correct answer. Any executable named param anywhere on PATH - last entry is enough, no precedence needed - that prints something containing OpenHarmony in response to get const.ohos.fullname will flip the result. I reproduced this in sh, bash, dash, and zsh: a macOS host reports openharmony instead of darwin. This isn't a privilege boundary (nvm already resolves uname/grep/curl through PATH, so PATH is inside the trusted base), it's robustness: param is a generic, unclaimed name on every non-OpenHarmony system, unlike the POSIX tool names, so a collision needs no compromise at all.
2. It costs two forks per call on every platform. The pipeline forks a subshell for the left side and execs grep on the right even when the PATH search for param fails. Measured here, 300 iterations under bash: nvm_get_os goes 264ms → 861ms with a mocked Linux uname, and 468ms → 1065ms on the real Darwin one - roughly +2ms/call, ~3x. nvm_get_os is uncached and sits on the shell-startup path (nvm_auto use → nvm use --silent → nvm_ensure_version_installed → nvm_is_version_installed), where there's already a comment reading "This nvm_ensure_version_installed call can be a performance bottleneck on shell startup." It's also once per alias in nvm ls/nvm alias on a colored TTY.
This is also inconsistent with nvm_get_arch immediately below, which gates every OS-specific probe on the detected OS - pkg_info/isainfo behind sunos, ppc64 behind aix, the /sbin/init check behind uname = Linux, and the alpine-musl mapping behind linux even though /etc/alpine-release is already conclusive.
Suggested replacement for this block:
| # OpenHarmony cannot be reliably detected via `uname` because the device | |
| # may run either a Linux kernel or a HarmonyOS kernel; detect it from the | |
| # userland instead. | |
| if command param get const.ohos.fullname 2>/dev/null | nvm_grep -q "OpenHarmony"; then | |
| NVM_OS=openharmony | |
| fi | |
| # OpenHarmony can't be identified from `uname` alone: a device may run either | |
| # a Linux kernel or a HarmonyOS kernel. Ask the userland, but only when | |
| # `uname` has not already identified some other OS. | |
| case "${NVM_OS-}" in | |
| '' | linux) | |
| if nvm_has param && command param get const.ohos.fullname </dev/null 2>/dev/null | nvm_grep -q 'OpenHarmony'; then | |
| NVM_OS=openharmony | |
| fi | |
| ;; | |
| esac |
I checked this is byte-for-byte equivalent to your version on all three cases in the new test (the HarmonyOS … mock matches no case arm, so NVM_OS is empty and the probe still runs), it rejects a param that isn't OpenHarmony's, it's nounset-safe, and it restores the original timing: 463ms vs 468ms pre-patch, against 1065ms as written.
Note this narrows the blast radius rather than eliminating it - a stray param on an actual Linux host would still flip linux → openharmony. If you can confirm the on-device path, gating on [ -x /system/bin/param ], or reading /system/etc/param/ohos_const/ohos.para directly and skipping the exec entirely, would close that too and cost nothing. I've only verified those paths from startup_init's BUILD.gn, not on real hardware, so that's your call. (FWIW the param binary target is only built for non-ohos_lite systems; lite systems expose the same thing as begetctl param get. Not worth handling - nvm needs a POSIX shell plus curl/wget and tar, which lite systems don't realistically provide.)
| # OpenHarmony cannot be reliably detected via `uname` because the device | ||
| # may run either a Linux kernel or a HarmonyOS kernel; detect it from the | ||
| # userland instead. | ||
| if command param get const.ohos.fullname 2>/dev/null | nvm_grep -q "OpenHarmony"; then |
There was a problem hiding this comment.
Two specifics behind the nvm_has guard and the </dev/null in my suggestion above.
command does not bypass zsh's command_not_found_handler. Verified under zsh -f:
$ zsh -f -c 'command_not_found_handler() { print -r -- "HANDLER FIRED: $*" >&2; return 127 }
command param get const.ohos.fullname | grep -q OpenHarmony'
HANDLER FIRED: param get const.ohos.fullname
The 2>/dev/null here hides the message but not the invocation or its cost - and Debian/Ubuntu, Arch, Fedora and NixOS all ship handlers that query a package database. Worse, a handler that writes to stdout feeds straight into nvm_grep:
$ zsh -f -c 'command_not_found_handler() { print -r -- "OpenHarmony-9.9"; return 127 }
command param get const.ohos.fullname 2>/dev/null | grep -q OpenHarmony && echo hijacked'
hijacked
nvm_has param (just the type builtin, no fork) makes the handler fire zero times; I confirmed that, and confirmed the probe still resolves correctly with your mocks in sh, bash, dash, zsh and ksh. command -v would also work - nvm_has just matches the surrounding style, cf. nvm_has sw_vers in nvm debug. I couldn't test bash ≥ 4 (only 3.2 available here), so I'm making no claim about command_not_found_handle.
The probe inherits stdin. Command substitution doesn't isolate it, so a param on PATH that reads stdin either eats the caller's input or blocks. Both reproduced: with a param doing cat >/dev/null, a subsequent read -r LINE in the calling script came back empty in bash and dash; with one doing read -r x, nvm_get_os was still blocked after 3 seconds. Since nvm_process_parameters → nvm_auto use → nvm use runs at source time, that would hang every new shell. </dev/null is cheap insurance - nvm doesn't do this anywhere else today, but nowhere else does it invoke a name this likely to be somebody's own script.
Last nit on this line: nvm_grep -q "OpenHarmony" matches the substring anywhere in any line. Anchoring to '^OpenHarmony' would be tighter, but I don't know whether every param build prints a bare value versus key = value, so I'd only do it if you've confirmed the output format on-device.
|
|
||
| die() { cleanup; echo "$@"; exit 1; } | ||
|
|
||
| . ../../../nvm.sh |
There was a problem hiding this comment.
Please match the convention the rest of the suite uses:
| . ../../../nvm.sh | |
| : nvm.sh | |
| \. ../../../nvm.sh |
On current master, 119 of the 120 unit tests that source nvm.sh use the backslash-escaped \. (the sole exception is nvm_get_arch_unofficial), and 107 of 124 carry the : nvm.sh marker line.
The escape isn't cosmetic - zsh reads ~/.zshenv even for non-interactive scripts, and alias expansion is on there, so a user with alias .='…' gets the alias instead of the source builtin. The : nvm.sh line sets $_ for NVM_SCRIPT_SOURCE="${_:-}" at the top of nvm.sh; it's inert in CI since the workflows export NVM_DIR, but it's what every sibling does.
| # plain Linux aarch64, no `param` on PATH | ||
| ln -sf "${MOCKS_DIR}/uname_linux_aarch64" ./uname | ||
| rm -f ./param | ||
| run_test "linux" |
There was a problem hiding this comment.
This case does rm -f ./param and then relies on the host having no param on PATH - removing the symlink doesn't shadow a real one further down. The host class where that's guaranteed to break is an actual OpenHarmony device, i.e. the platform this PR adds support for: nvm_get_os returns openharmony against an expected linux and the test dies.
More importantly, there's no case at all for the branch this whole heuristic rests on: a param that exists but isn't OpenHarmony's. The repo already has the pattern for this - test/mocks/pkg_info_fail. Please add a test/mocks/param_other:
if [ "$#" -eq 2 ] && [ "$1" = "get" ] && [ "$2" = "const.ohos.fullname" ]; then
echo "SomeOtherOS-1.2.3"
fiand use it here instead of rm -f:
| # plain Linux aarch64, no `param` on PATH | |
| ln -sf "${MOCKS_DIR}/uname_linux_aarch64" ./uname | |
| rm -f ./param | |
| run_test "linux" | |
| # plain Linux aarch64, with an unrelated `param` on PATH | |
| ln -sf "${MOCKS_DIR}/uname_linux_aarch64" ./uname | |
| ln -sf "${MOCKS_DIR}/param_other" ./param | |
| run_test "linux" |
That both makes the case hermetic and pins the no-false-positive property. Keep a rm -f ./param variant too if you want to cover the genuinely-absent path.
| ln -sf "${MOCKS_DIR}/param_openharmony" ./param | ||
| run_test "openharmony" | ||
|
|
||
| cleanup |
There was a problem hiding this comment.
Since this is the first unit test nvm_get_os has ever had, it'd be good to lock in more than the two arms the PR touches. Three additions, all cheap:
-
HarmonyOS
unamewith noparam- asserts that the new mock alone isn't what producesopenharmony, and pins the empty-string result:# HarmonyOS kernel without the OpenHarmony userland ln -sf "${MOCKS_DIR}/uname_harmonyos_aarch64" ./uname rm -f ./param run_test ""
-
Non-Linux host with
parampresent - this is the case my other comment is about; once the probe is gated it should assertdarwin, which is exactly the regression test for it:ln -sf "${MOCKS_DIR}/uname_osx_amd64" ./uname ln -sf "${MOCKS_DIR}/param_openharmony" ./param run_test "darwin"
-
The pre-existing arms -
uname_osx_amd64anduname_smartos_amd64already exist intest/mocks, sodarwinandsunosare oneln -sfeach. Right now five ofnvm_get_os's sevencasearms are untested.
Also worth knowing (not a defect, just a footnote): urchin expands the directory glob once before running, so the transient ./uname and ./param symlinks can't be picked up as tests mid-run. But a run interrupted between ln -sf ./param and cleanup leaves a ./param behind, and since . is on PATH for this directory, every later nvm_get_os call in it would report openharmony. Your cleanup is already stricter than nvm_get_arch's about this; just something to keep in mind if a local run ever goes weird.
342800b to
215b991
Compare
|
@ljharb Thank you for the review. I have revised the PR based on your feedback, can you review it again? Regarding the open questions, here is what I have confirmed:
In this commit, I made the following changes:
|
|
Some tests failed and are unrelated to my changes, which looks to be caused by CI infrastructure instability. |
7b1d4bf to
addc7a4
Compare
| @@ -0,0 +1,3 @@ | |||
| if [ "$#" -eq 2 ] && [ "$1" = "get" ] && [ "$2" = "const.ohos.fullname" ]; then | |||
| echo "SomeOtherOS-1.2.3" | |||
There was a problem hiding this comment.
Worth making this NotOpenHarmony-1.2.3. As-is it shares no substring with OpenHarmony, so dropping the ^ from nvm_grep -q '^OpenHarmony' leaves all 8 assertions green. Changing the string keeps both existing expectations and makes them pin the anchor.
| # under /lib64, coreutils binaries). On musl Alpine that setup does not | ||
| # apply, and nvm_get_os's OpenHarmony probe cannot be exercised there; | ||
| # skip the chroot checks on Alpine. | ||
| if [ -f "/etc/alpine-release" ]; then |
There was a problem hiding this comment.
This skips the whole block on every Alpine lane, and the four arms that remain pass byte-identically with the entire nvm.sh hunk deleted. 24 of the 32 fast jobs assert nothing about this PR, including the only arm64 coverage.
One arm closes most of that without a chroot, so it runs everywhere:
# openharmony + aarch64 composes the slug a mirror has to serve
ln -sf "${MOCKS_DIR}/uname_linux_aarch64" ./uname
nvm_get_os() { nvm_echo openharmony; }
SLUG="$(nvm_get_download_slug node binary v22.22.0)"
[ "${SLUG}" = 'node-v22.22.0-openharmony-arm64' ] || die "nvm_get_download_slug produced \"${SLUG}\""
unset nvm_get_os
rm -f ./unameChecked in sh, bash, dash and zsh. It also pins that nvm_get_arch gives arm64 and not arm64-musl here, which the Alpine lanes are the only ones that could get wrong.
| OS_UNAME_HARMONYOS_PARAM_OTHER=$(sudo chroot "${CHROOT_UNAME_HARMONYOS_PARAM_OTHER}" /bin/sh -c ". ./nvm.sh && nvm_get_os") | ||
| if [ "_${OS_UNAME_HARMONYOS_PARAM_OTHER}" != "_" ]; then | ||
| die "nvm_get_os produced \"${OS_UNAME_HARMONYOS_PARAM_OTHER}\", expected \"\"" | ||
| fi |
There was a problem hiding this comment.
Nothing pins the '' | linux guard: delete the case wrapper so the probe runs unconditionally, and all 8 assertions still pass. That is the property the last round asked for.
One more fixture covers it:
CHROOT_UNAME_OSX_PARAM_OPENHARMONY="${TMP_DIR}/uname_osx_param_openharmony"
setup_chroot "${CHROOT_UNAME_OSX_PARAM_OPENHARMONY}" "${MOCKS_DIR}/param_openharmony" "${MOCKS_DIR}/uname_osx_amd64"
# an OS uname already identified is never re-probed
OS_UNAME_OSX_PARAM_OPENHARMONY=$(sudo chroot "${CHROOT_UNAME_OSX_PARAM_OPENHARMONY}" /bin/sh -c ". ./nvm.sh && nvm_get_os")
if [ "${OS_UNAME_OSX_PARAM_OPENHARMONY}" != "darwin" ]; then
die "nvm_get_os produced \"${OS_UNAME_OSX_PARAM_OPENHARMONY}\", expected \"darwin\""
fi| EXPECTED=$1 | ||
| local OUTPUT | ||
| OUTPUT="$(nvm_get_os)" | ||
| [ "_${OUTPUT}" = "_${EXPECTED}" ] || |
There was a problem hiding this comment.
die runs before the caller reaches cleanup_mock_os, and cleanup has no rm -f ./uname, so a failing arm leaves an executable symlink in this directory; urchin then runs it as a test on the next pass. nvm_get_arch tears down inside run_test, nvm_get_arch alpine puts rm -f ./uname in cleanup. Either is fine.
2 participants
Description
Node.js currently supports the OpenHarmony platform (see BUILDING.md), but
nvmdoes not yet support it. Therefore, I am submitting this patch to add OpenHarmony platform support tonvm.Proposed Changes
OpenHarmony has various distributions featuring mutually compatible user spaces but different underlying kernels, meaning the same binary may run on different kernels. Currently, the industry primarily uses Linux and HarmonyOS kernels. Consequently, we cannot simply rely on
unameto identify the platform. Instead, platform detection is implemented using user-space characteristic files and strings.Distribution Source
Since the support tier for OpenHarmony in Node.js is currently marked as "Experimental", the official distribution source nodejs.org/dist does not publish OpenHarmony binaries. Users must specify a third-party source to install OpenHarmony builds via
nvm.For example, ohos-node.com/dist is an available third-party source. This source is currently maintained by me. You can visit the project homepage for more information.
I have verified that after running
export NVM_NODEJS_ORG_MIRROR="https://ohos-node.com/dist",nvmcan successfully fetch OpenHarmony binaries from this source.Support
I am willing to actively participate in maintaining OpenHarmony support for
nvm. If users submit OpenHarmony-related issues tonvmin the future, feel free to mention me or send me an email, and I will be happy to provide assistance.