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

Process additional (--) parameters before final version parameter. #1957

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@leebickmtu
Copy link

commented Dec 12, 2018

Fixes issue #1762.

Prior behavior:
nvm install --lts --reinstall-packages-from=8.9.3
Failed with error:

grep: unrecognized option `--reinstall-packages-from=8.9.3'
usage: grep [-abcDEFGHhIiJLlmnOoqRSsUVvwxZ] [-A num] [-B num] [-C[num]]
	[-e pattern] [-f file] [--binary-files=value] [--color=when]
	[--context[=num]] [--directories=action] [--label] [--line-buffered]
	[--null] [pattern] [file ...]
Version '--reinstall-packages-from=8.9.3' (with LTS filter) not found - try `nvm ls-remote --lts` to browse available versions.

Fixed behavior:
nvm install --lts --reinstall-packages-from=8.9.3
Succeeds!

The install command 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 --lts flag, 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...

Version '--reinstall-packages-from=8.9.3' (with LTS filter) not found

@leebickmtu

This comment has been minimized.

Copy link
Author

commented Dec 12, 2018

Also, while this patch will solve the immediate issue, the existing behavior of handling parameters as 2 groups is less than ideal.
It makes mixing the parameters together in order not possible.

For example this would not work (due to having a param from group 2 between params from group 1):
nvm install --lts --reinstall-packages-from=8.9.3 --latest-npm
But this would work:
nvm install --lts --latest-npm --reinstall-packages-from=8.9.3

Parameter order should not matter.
I'm happy with merging this fix as is, since this limitation already existed, but will probably revisit to correct the whole system in the future.

@ljharb
Copy link
Collaborator

left a comment

It'd be great to include some regression tests, as well :-)

nvm.sh Outdated
ADDITIONAL_PARAMETERS="$ADDITIONAL_PARAMETERS $1"
;;
esac
shift

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 12, 2018

Collaborator

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

This comment has been minimized.

Copy link
@leebickmtu

leebickmtu Dec 13, 2018

Author

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.

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 13, 2018

Collaborator

Let's not be so hasty; I'm sure there's some way it can be addressed, it just might not be super clean.

@leebickmtu

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

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.

@leebickmtu leebickmtu closed this Dec 13, 2018

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2018

I'm going to reopen this; i think this is still worth exploring.

@ljharb ljharb reopened this Dec 13, 2018

Bickel, Korbin added some commits Dec 13, 2018

Bickel, Korbin
Combine all parameters into single case. Removes all order dependance…
… (except optional compile params, which must come after all install params in order to not be confused with the optional version param).
Bickel, Korbin
Parse parameter in more straight forward way. Add failing case for mi…
…ssing value on alias --copy-packages-from same as on --reinstall-packages-from.
Bickel, Korbin
Bickel, Korbin
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

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 13, 2018

Collaborator

it looks like the job count is now consumed into ADDITIONAL_PARAMETERS?

This comment has been minimized.

Copy link
@leebickmtu

leebickmtu Dec 14, 2018

Author

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.

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 15, 2018

Collaborator

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.