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

nvm install xx should install latest minor version (xx.*) with this major number #596

Closed
Mart-Bogdan opened this issue Dec 18, 2020 · 5 comments

Comments

@Mart-Bogdan
Copy link

When I run nvm install 10 it downloads 10.0.0 version instead of downloading latest 10.*.

Expected behavior: download 10.23.0
Current behavior: download 10.0.0

and downloading 0 releases from major would have lots of bugs.

This isn't logical and this controversy with UNIX nvm behavior

Linx NVM is working correctly with semver

$ nvm install 10
Downloading and installing node v10.23.0...
Downloading https://nodejs.org/dist/v10.23.0/node-v10.23.0-linux-x64.tar.xz...
@Serializator
Copy link

It seems that this should be the case already but isn't working properly. I installed Node 14 using nvm install 14 and indeed it installed 14.0.0 not 14.15.3 as mentioned on nodejs.org.

nvm-windows/src/nvm.go

Lines 193 to 199 in 0619023

// if the user specifies only the major version number then install the latest
// version of the major version number
if len(version) == 1 {
version = findLatestSubVersion(version)
} else {
version = cleanVersion(version)
}

If I read the code correctly it seems that in case of nvm install 14 that version is 14, thus a length of 2 and not 1 (which makes the conditional to invoke findLatestSubVersion truthy).

Within cleanVersion(version string) a regular expression is used to determine the major, minor and patch given a version. What about using this to determine whether the end-user gave only a major version or not?

nvm-windows/src/nvm.go

Lines 628 to 644 in 0619023

func cleanVersion(version string) string {
re := regexp.MustCompile("\\d+.\\d+.\\d+")
matched := re.FindString(version)
if len(matched) == 0 {
re = regexp.MustCompile("\\d+.\\d+")
matched = re.FindString(version)
if len(matched) == 0 {
matched = version + ".0.0"
} else {
matched = matched + ".0"
}
fmt.Println(matched)
}
return matched
}

I'm still in doubt whether this behavior is desired at all. If an end-user specifies both the major and minor version it (in my opinion) should still get the latest patch version. So... Is the conditional which is there at the moment even necessary?

Please let me know if I misinterpreted something, either in the description of the issue or the code!

@Serializator
Copy link

I should've looked through other issues before commenting. I believe this to be a duplicate of #156, which describes the same behavior within it.

@Mart-Bogdan
Copy link
Author

There are module semver inside nvmw https://github.com/coreybutler/nvm-windows/blob/master/src/nvm/semver/semver.go#L184 but it seems is able to parse only full version:

  // Split into major.minor.(patch+pr+meta)
  parts := strings.SplitN(s, ".", 3)
  if len(parts) != 3 {
    return nil, errors.New("No Major.Minor.Patch elements found")
  }

IDK, perhap package Masterminds/semver could be used? It's Constrainsts seems to cover this: https://github.com/Masterminds/semver#checking-version-constraints

But actually changing len with regex should do the work too.

Regarding #156 -- it seems reporting a crash if not full version is given, which is seems resolved.

@Serializator
Copy link

#156 was originally created for the crash you mentioned. Reading though I also see talk about whether nvm install 4 installs the latest minor and patch version or not and whether it should or not.

@coreybutler
Copy link
Owner

Closing as a duplicate.

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

No branches or pull requests

3 participants