Process additional (--) parameters before final version parameter. #1957
Conversation
|
Also, while this patch will solve the immediate issue, the existing behavior of handling parameters as 2 groups is less than ideal. For example this would not work (due to having a param from group 2 between params from group 1): Parameter order should not matter. |
nvm.sh
Outdated
| ADDITIONAL_PARAMETERS="$ADDITIONAL_PARAMETERS $1" | ||
| ;; | ||
| esac | ||
| shift |
There was a problem hiding this comment.
this shift will change what $1 is in the next few lines; i think it needs to be extracted first, otherwise it will always end up in ADDITIONAL_PARAMETERS
There was a problem hiding this comment.
This shift is fine. The param only ends up in ADDITIONAL_PARAMETERS if it didn't match the above cases. I believe ADDITIONAL_PARAMETERS gathers all the unknown params and passes them to the nvm_install_source function for use there to customize the binary compile.
However that does present a problem, since my fix is to make the version specifier optional when --lts is used. But it wouldn't be able to be differentiated from these also optional compile params.
I forgot about the optional compile params. Basically the install command is fucked unless we are willing to break the current way people are using it. As I see it we can't cleanly fix the --lts use case where no version is given without breaking params after it. Closing PR now.
There was a problem hiding this comment.
Let's not be so hasty; I'm sure there's some way it can be addressed, it just might not be super clean.
|
Found complexities not originally accounted for with optional binary compile params. Since there would be 2 optional param groups, this cannot be done without breaking existing use of the command. |
|
I'm going to reopen this; i think this is still worth exploring. |
… (except optional compile params, which must come after all install params in order to not be confused with the optional version param).
…ssing value on alias --copy-packages-from same as on --reinstall-packages-from.
| nvm_get_make_jobs "$1" | ||
| shift # consume job count | ||
| shift # 2 part parameter, comsume first part now | ||
| nvm_get_make_jobs "$1" # $1 is now second parameter piece |
There was a problem hiding this comment.
it looks like the job count is now consumed into ADDITIONAL_PARAMETERS?
There was a problem hiding this comment.
ADDITIONAL_PARAMETERS is only added to if the parameter does not meet the other cases above it. The job count parameter is it's own case. So it is not added to ADDITIONAL_PARAMETERS.
There was a problem hiding this comment.
which line handles that? note that nvm install and nvm use, with no "provided version" argument (but still with possible additional parameters) are also valid use case (they'll read from .nvmrc)
|
@leebickmtu are you still interested in completing this PR? |
|
@leebickmtu id prefer if you could restore the branch and reopen it; that way i can finish it. |
Fixes issue #1762.
Prior behavior:
nvm install --lts --reinstall-packages-from=8.9.3Failed with error:
Fixed behavior:
nvm install --lts --reinstall-packages-from=8.9.3Succeeds!
The
installcommand code separates processing parameters into 2 groups.Main group
[-s, -j, --no-progress, --lts, --lts=*, --latest-npm]Additional group
[--reinstall-packages-from=*, --copy-packages-from=*, --skip-default-packages]The issue is that between processing these groups it was also grabbing a parameter interpreted as a version# to be installed. So when using the
--ltsflag, where then this version paramter won't be present if any of the params from the additional group where present it grabbed the first one as a version thus giving an error like...