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

fix: nodebuild call failure after initial install #409

Closed
wants to merge 2 commits into from

Conversation

mkungla
Copy link

@mkungla mkungla commented Feb 3, 2025

Description:

From asdf version v0.16.0, this plugin breaks and fails when trying to install a Node version. This PR introduces a fix that addresses this issue for asdf >= v0.16.0 while ensuring backward compatibility with older versions of asdf.

Changes Introduced:

  • Fixes compatibility with asdf v0.16.0 and later versions.
  • Ensures no breaking changes for versions prior to v0.16.0.

Testing:

  • Tested with asdf v0.16.0 and v0.15.0 to verify proper functionality.
  • Maintains compatibility with earlier versions of asdf.

Closes #407
Closes #412
Closes #414
Closes #415

@queue-tip
Copy link

I opened #407 yesterday. It looks like it's related.

@fabiog27
Copy link

#414 would also be fixed by this I think.

@Stratus3D
Copy link
Member

@augustobmoura can you take a look at this one? I think it's a good fix to accommodate asdf 0.16.0+.

@mkungla
Copy link
Author

mkungla commented Feb 11, 2025

plugin test should be now passing and this pr closes also folllowing issues #407, #412, #414, #415

@skull-squadron
Copy link

skull-squadron commented Mar 2, 2025

Tested manually. Works (albeit with filename extension changes). Please fix and merge because asdf cmd nodejs update-nodebuild is currently broken.

@mkungla
Copy link
Author

mkungla commented Mar 2, 2025

@Stratus3D and @augustobmoura How is this repository maintained that such simple and obvious thing takes so long to merge?

@augustobmoura
Copy link
Member

@Stratus3D and @augustobmoura How is this repository maintained that such simple and obvious thing takes so long to merge?

The maintainers have jobs and lives outside this project. And given the start of the year and the amount of international holidays, nobody is expected to work on a schedule.

The bugs have workaround and you can always pin/downgrade asdf if that is bothering you. To be frank, I didn't even hit any of the problems because the plugin is pretty stable and I didn't upgrade asdf in the last months.

Copy link
Member

@augustobmoura augustobmoura left a comment

Choose a reason for hiding this comment

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

Left a few comments, and BTW, I cannot hit the issue on asdf 0.16.3, are you sure this fix is even needed?

@@ -4,8 +4,28 @@ set -eu -o pipefail

source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../utils.sh"

: "${ASDF_NODEJS_NODEBUILD_HOME=$ASDF_NODEJS_PLUGIN_DIR/.node-build}"
: "${ASDF_NODEJS_CONCURRENCY=$(((${ASDF_CONCURRENCY:-1} + 1) / 2))}"
Copy link
Member

Choose a reason for hiding this comment

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

Explanation of the problem:

ASDF_CONCURRENCY was being defaulted here to 1, so I don't think the problem was ASDF_CONCURRENCY being unbound. But we always assumed it to be a number. Because it can now be defined to auto, bash tries to interpolate it as $((auto + 1 / 2)), which surprises me, I didn't know bash tries to interpolate variables in arithmetic expressions without the dollar sign.

Comment on lines +11 to +14
if [[ "${local_concurrency:-auto}" == "auto" || -z "${local_concurrency}" ]]; then
if command -v nproc >/dev/null 2>&1; then
local_concurrency=$(nproc)
elif [[ "$(uname)" == "Darwin" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Double equals == and double brackets [[ are bashisms, please avoid them, we try to keep the project as compatible to POSIX shell as possible

local_concurrency can not be empty since you are already setting it to something on line 8. We can simplify the first if condition further.

Suggested change
if [[ "${local_concurrency:-auto}" == "auto" || -z "${local_concurrency}" ]]; then
if command -v nproc >/dev/null 2>&1; then
local_concurrency=$(nproc)
elif [[ "$(uname)" == "Darwin" ]]; then
if [ "$local_concurrency" = auto ]; then
if command -v nproc >/dev/null 2>&1; then
local_concurrency=$(nproc)
elif [ "$(uname)" = Darwin ]; then

fi

# Ensure ASDF_NODEJS_PLUGIN_DIR is set
ASDF_NODEJS_PLUGIN_DIR="${ASDF_NODEJS_PLUGIN_DIR:-$HOME/.asdf/plugins/nodejs}"
Copy link
Member

Choose a reason for hiding this comment

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

NodeJS plugin home can change based on ASDF_DIR and ASDF_DATA_DIR, I prefer to just reference the local file, like in

export ASDF_NODEJS_PLUGIN_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)

This variable should be defined anyway, since we are sourcing ASDF_CONCURRENCY on line 5. We can remove this line here:

Suggested change
ASDF_NODEJS_PLUGIN_DIR="${ASDF_NODEJS_PLUGIN_DIR:-$HOME/.asdf/plugins/nodejs}"

Copy link
Member

Choose a reason for hiding this comment

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

I agree. If possible hardcoding paths should be avoided in plugins.

@augustobmoura
Copy link
Member

In my opinion this should be fixed by asdf-core, we shouldn't expect to resolve auto on every plugin

@Stratus3D
Copy link
Member

In my opinion this should be fixed by asdf-core, we shouldn't expect to resolve auto on every plugin

Sorry all, been behind on PRs recently. @augustobmoura you are correct. This should not be the responsibility of this plugin. asdf core should handle this.

In my opinion this ASDF_CONCURRENCY feature is more hassle than it's worth. It's pretty easy for plugins to figure out a fairly optimal setting on their own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants