fix(aqua): align version resolution logic in list_bin_paths #5562

risu729 · 2025-07-11T01:56:01Z

Aligns the version resolution logic (from canonicalised version to GitHub tag) in list_bin_paths with the one in install_version_.

I think this doesn't change the behaviour for offline introduced in #5195.

TODO: instead of caching it would probably be better to create this as part of installation

Regarding this TODO, I agreed with it and tried to work on it, but was not sure how to.
I'd like to ask for some advice.

I thought of storing bin_paths.msgpack.z in the installation path, tv.install_path(), while the installation, and reading it when list_bin_paths is called.
Not to break the installations with old versions, or just in case bin_paths.msgpack.z is removed somehow, I think we need to fall back to the current implementation if it's not found.

However, then it would be the same as the current implementation, except that list_bin_paths is called in the installation and the cache is created.

Copilot

Pull Request Overview

This PR refactors the version resolution logic in the Aqua backend by extracting duplicated code into a shared resolve_version method. The change aligns the version resolution logic between install_version_ and list_bin_paths methods to ensure consistency.

Key changes:

Extracted version resolution logic into a new resolve_version method
Updated both install_version_ and list_bin_paths to use the shared implementation
Simplified the srcs method to use resolved version information directly

Copilot · 2025-07-11T08:04:12Z

src/backend/aqua.rs

+            .fetch_url(&pkg, v_prefixed.as_ref().unwrap_or(&v))
+            .await
+        {
+            Ok(url) => (url, v_prefixed.unwrap_or(v)),


The use of unwrap_or(v) here consumes v by value, but on line 210 v is used again as v.as_str(). This creates an inconsistency where v is moved in the success case but borrowed in the error case. Consider using v_prefixed.as_ref().unwrap_or(&v).clone() to maintain consistency.

Suggested change

Ok(url) => (url, v_prefixed.unwrap_or(v)),

Ok(url) => (url, v_prefixed.as_ref().unwrap_or(&v).clone()),

Copilot · 2025-07-11T08:04:13Z

src/backend/aqua.rs

-                        let dst = src.parent().unwrap().join(f.name.as_str());
-                        (src, dst)
-                    }))
+                Ok(f.src(pkg, v)?.map(|src| install_path.join(src)).map(|src| {


The removal of the unique_by filter on line 739 (in the removed code) means that duplicate source paths may now be processed multiple times. This could lead to redundant operations or unexpected behavior if the same file is processed multiple times.

jdx · 2025-07-11T08:17:38Z

I probably wrote that TODO but I'm not sure I agree with it. Manifest files like .backend.meta have often caused issues because they don't get updated on new releases and mise users should be able to manually create tools which you called out as the problem here, we'd need the fallback anyways.

jdx · 2025-07-11T14:45:18Z

@risu729 I am pretty sure this PR caused some serious performance degradation. You can see this in the hyperfine check. I'll need to revert unfortunately.

…5562)" This reverts commit 5208775.

…5574) Reverts #5562

@risu729

### 🐛 Bug Fixes - **(aqua)** align version resolution logic in list_bin_paths by [@risu729](https://github.com/risu729) in [#5562](#5562) - Xonsh integration by [@jfmontanaro](https://github.com/jfmontanaro) in [#5557](#5557) ### 📚 Documentation - create comprehensive architecture documentation suite and enhance development guides by [@jdx](https://github.com/jdx) in [d2b4a05](d2b4a05) ### 📦️ Dependency Updates - update rust crate bzip2 to 0.6 by [@renovate[bot]](https://github.com/renovate[bot]) in [#5568](#5568) - update rust crate clap_mangen to v0.2.28 by [@renovate[bot]](https://github.com/renovate[bot]) in [#5566](#5566) - update rust crate clap to v4.5.41 by [@renovate[bot]](https://github.com/renovate[bot]) in [#5565](#5565) - update rust crate taplo to 0.14 by [@renovate[bot]](https://github.com/renovate[bot]) in [#5158](#5158) ### Chore - added xonsh for release builds by [@jdx](https://github.com/jdx) in [#5561](#5561) - enable backtrace lines on panic by [@jdx](https://github.com/jdx) in [#5571](#5571) - shfmt update by [@jdx](https://github.com/jdx) in [67ee245](67ee245) ### New Contributors - @jfmontanaro made their first contribution in [#5557](#5557)

risu729 · 2025-07-12T06:37:04Z

My apologies, I should've marked it as a draft.

I thoght it was because it accesses GitHub API to list bin paths, but not really sure why it also slowed down the install itself.

risu729 · 2025-07-12T06:38:17Z

I probably wrote that TODO but I'm not sure I agree with it. Manifest files like .backend.meta have often caused issues because they don't get updated on new releases and mise users should be able to manually create tools which you called out as the problem here, we'd need the fallback anyways.

Yeah, I think it might cause some issues, but I think this is necessary not only for the offline usage but also for the performance.

jdx · 2025-07-12T10:00:38Z

I don't see an obvious place here where this would be the case but usually it's because an eyre error is being built (even if it isn't raised) that takes a surprising amount of time.

risu729 · 2025-07-12T13:33:10Z

thanks, yeah it could be the cause. I'll try to investigate that

@yjoer

risu729 added 2 commits July 11, 2025 11:43

fix(aqua): align version resolution logic in list_bin_paths

f41b15c

refactor: use Path instead of PathBuf

53be976

jdx requested a review from Copilot July 11, 2025 08:03

Copilot AI reviewed Jul 11, 2025

View reviewed changes

jdx merged commit 5208775 into jdx:main Jul 11, 2025
18 checks passed

mise-en-dev mentioned this pull request Jul 11, 2025

chore: release 2025.7.4 #5560

Merged

jdx added a commit that referenced this pull request Jul 11, 2025

Revert "fix(aqua): align version resolution logic in list_bin_paths (#…

b3f617a

…5562)" This reverts commit 5208775.

jdx mentioned this pull request Jul 11, 2025

Revert "fix(aqua): align version resolution logic in list_bin_paths" #5574

Merged

jdx added a commit that referenced this pull request Jul 11, 2025

Revert "fix(aqua): align version resolution logic in list_bin_paths" (#…

c073ae2

…5574) Reverts #5562

risu729 deleted the refactor-aqua-bin-paths branch July 11, 2025 15:28

BrewTestBot mentioned this pull request Jul 12, 2025

mise 2025.7.4 Homebrew/homebrew-core#229839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(aqua): align version resolution logic in list_bin_paths #5562

fix(aqua): align version resolution logic in list_bin_paths #5562

Uh oh!

risu729 commented Jul 11, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 11, 2025

Uh oh!

Copilot AI Jul 11, 2025

Uh oh!

jdx commented Jul 11, 2025

Uh oh!

Uh oh!

jdx commented Jul 11, 2025

Uh oh!

risu729 commented Jul 12, 2025

Uh oh!

risu729 commented Jul 12, 2025

Uh oh!

jdx commented Jul 12, 2025

Uh oh!

risu729 commented Jul 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	Ok(url) => (url, v_prefixed.unwrap_or(v)),
	Ok(url) => (url, v_prefixed.as_ref().unwrap_or(&v).clone()),

Uh oh!

fix(aqua): align version resolution logic in list_bin_paths #5562

fix(aqua): align version resolution logic in list_bin_paths #5562

Uh oh!

Conversation

risu729 commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

jdx commented Jul 11, 2025

Uh oh!

Uh oh!

jdx commented Jul 11, 2025

Uh oh!

risu729 commented Jul 12, 2025

Uh oh!

risu729 commented Jul 12, 2025

Uh oh!

jdx commented Jul 12, 2025

Uh oh!

risu729 commented Jul 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

risu729 commented Jul 11, 2025 •

edited

Loading