[BE-1996] Support .nvmrc version files for tool setup#1233
[BE-1996] Support .nvmrc version files for tool setup#1233marcell-vida wants to merge 2 commits intomasterfrom
.nvmrc version files for tool setup#1233Conversation
SummaryAdded support for Walkthrough
|
![bitrise[bot]](https://avatars.githubusercontent.com/in/32878?s=60&v=4){kind=small}
| return nil, fmt.Errorf("%s: invalid version (empty after removing 'v' prefix)", path) | ||
| } | ||
| return []ToolVersion{ | ||
| {ToolName: "node", Version: version}, |
There was a problem hiding this comment.
🐛 Bug
The ToolName is hardcoded as "node", but the alias map in toolprovider/alias/alias.go shows that "node" is an alias for "nodejs". Other parsers like parseSingleToolVersion use inferToolID which calls GetCanonicalToolID to convert aliases to canonical tool IDs. This creates an inconsistency: .node-version files return "nodejs" (canonical), while .nvmrc files return "node" (alias). While the alias conversion happens later in run.go, the parser should be consistent with other parsers and return the canonical tool ID "nodejs".
🔄 Suggestion:
| {ToolName: "node", Version: version}, | |
| {ToolName: "nodejs", Version: version}, |
toolprovider/mise/install.go
Outdated
| return false | ||
| } | ||
|
|
||
| if strings.HasPrefix(miseVersion, "v2026") { |
There was a problem hiding this comment.
🐛 Bug
Incomplete version check: the condition only excludes versions starting with "v2026" but doesn't account for future versions like "v2027", "v2028", etc. Additionally, it doesn't verify the patch version for v2026 (the fix was in v2026.3.10, so v2026.0.0 through v2026.3.9 would still need the workaround).
🤖 Prompt for AI Agents:
In toolprovider/mise/install.go at line 104, Improve the version check to properly handle all mise versions >= v2026.3.10 and future versions. Consider using semantic version comparison or a more robust string comparison.
There was a problem hiding this comment.
Plan is to remove this temporary change in 2 weeks and previously there were only 2025 Mise versions pinned. Since now we pin the latest (from 2026) it doesn't matter which exact date it is, since all 2026 editions will be correct after this.
| require.NoError(t, err) | ||
|
|
||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| tool: "node", | ||
| version: "22.22.1", | ||
| name: "Install non-core tool with forced nixpkgs backend", | ||
| tool: "fake-unsupported-tool", |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| assert.Contains(t, output, "3.29.0") | ||
| }, | ||
| }, | ||
| // fvm_config.json tests |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| InstallDir() string | ||
| RunMise(args ...string) (string, error) | ||
| RunMiseWithTimeout(timeout time.Duration, args ...string) (string, error) | ||
| RunMiseWithTimeoutAndEnvs(timeout time.Duration, extraEnvs map[string]string, args ...string) (string, error) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| // ShouldTryStableFallback checks if we should retry a Flutter installation without the -stable suffix. | ||
| // Returns the fallback version to try, or empty string if no fallback should be attempted. | ||
| func ShouldTryStableFallback( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
marcell-vida
changed the title
.nvmrc version files for tool setup.nvmrc version files for tool setup, fix Python and Flutter installs
toolprovider/run.go
Outdated
| var toolErr provider.ToolInstallError | ||
| if errors.As(err, &toolErr) { | ||
| // Try Flutter -stable suffix workaround for mise provider | ||
| if miseProvider, ok := toolProvider.(*mise.MiseToolProvider); ok { |
There was a problem hiding this comment.
Can we move this whole thing into the mise package? This is breaking the abstraction.
toolprovider/run.go
Outdated
| if errors.As(err, &toolErr) { | ||
| // Try Flutter -stable suffix workaround for mise provider | ||
| if miseProvider, ok := toolProvider.(*mise.MiseToolProvider); ok { | ||
| fallbackVersion, fallbackErr := workarounds.ShouldTryStableFallback(miseProvider.ExecEnv, toolErr, silent) |
There was a problem hiding this comment.
nit: the name of this method no longer reflects what is actually does (returns with a version number or an err). My broader concern is what I wrote above about parsing the error string, but if we decide to keep parsing the error string, we could at least rename this method to something more accurate.
marcell-vida
force-pushed
the
be-1996-nvmrc-support
branch
from
5edfcc6 to
fa1e27f
Compare
marcell-vida
changed the title
.nvmrc version files for tool setup, fix Python and Flutter installs.nvmrc version files for tool setup
| files := []struct { | ||
| directory string | ||
| filename string | ||
| }{{"", ".tool-versions"}, {"", ".ruby-version"}, {"", ".node-version"}, {"", ".fvmrc"}, {".fvm", "fvm_config.json"}} |
There was a problem hiding this comment.
We might want to .nvmrc to this list
| return nil, fmt.Errorf("read %s: %w", path, err) | ||
| } | ||
|
|
||
| content = bytes.TrimSpace(content) |
There was a problem hiding this comment.
nit: why not use strings.TrimSpace()?
2 participants
Checklist
README.mdis updated with the changes (if needed)go mod tidyboth in root and inintegrationtests/if any dependencies have changed.Version
Requires a MAJOR/MINOR/PATCH version update
Context
Changes
Investigation details
Decisions