-
-
Notifications
You must be signed in to change notification settings - Fork 769
feat(http): Add 'format' to http backend #6957
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
Conversation
|
bugbot run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
|
Whoops, forgot I had some local changes, let me revert that 😅 |
a6ee03a to
51ad75b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a format configuration option to the HTTP backend, allowing users to explicitly specify archive formats when URLs lack proper file extensions or have incorrect ones.
Key Changes:
- Added
formatconfiguration support with platform-specific overrides - Modified archive format detection to use the specified format when provided
- Added documentation and E2E tests for the new feature
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/backend/http.rs | Implements format detection override by appending the specified format as an extension to the file path for proper archive type detection |
| e2e/backend/test_http_format | Adds E2E tests covering both explicit format specification and platform-specific format configuration |
| docs/dev-tools/backends/http.md | Documents the new format option with examples for both global and platform-specific usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let current_ext = file_path.extension().and_then(|e| e.to_str()).unwrap_or(""); | ||
| let new_ext = if current_ext.is_empty() { | ||
| added_extension.clone() | ||
| } else { | ||
| format!("{}.{}", current_ext, added_extension) | ||
| }; | ||
| file_path.set_extension(new_ext); |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for combining extensions may produce incorrect results. If the file already has a valid extension like 'tar.gz' and the user specifies format='tar.gz', this would result in 'tar.gz.tar.gz'. Consider validating that the specified format isn't already present in the extension before appending it.
| let current_ext = file_path.extension().and_then(|e| e.to_str()).unwrap_or(""); | |
| let new_ext = if current_ext.is_empty() { | |
| added_extension.clone() | |
| } else { | |
| format!("{}.{}", current_ext, added_extension) | |
| }; | |
| file_path.set_extension(new_ext); | |
| let file_name = file_path.file_name().and_then(|n| n.to_str()).unwrap_or(""); | |
| // Only append the extension if it's not already present at the end of the file name | |
| if !file_name.ends_with(&format!(".{}", added_extension)) { | |
| let current_ext = file_path.extension().and_then(|e| e.to_str()).unwrap_or(""); | |
| let new_ext = if current_ext.is_empty() { | |
| added_extension.clone() | |
| } else { | |
| format!("{}.{}", current_ext, added_extension) | |
| }; | |
| file_path.set_extension(new_ext); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was arguably intentional for simplicity sake. Trying to "maybe replace" the extension is error-prone (since files like foo-1.0.0 have the extension .0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Format Omission Corrupts Cache Integrity
The cache key generation doesn't include the format option, which can cause cache collisions when the same file is used with different format specifications. If two tools use identical files but specify different format values (like tar.gz vs zip vs no format), they'll share the same cache entry even though they require different extraction methods, resulting in incorrect installations.
src/backend/http.rs#L43-L60
Lines 43 to 60 in 3c978d7
| /// Generate a cache key based on the actual file content (checksum) and extraction options | |
| fn get_file_based_cache_key( | |
| &self, | |
| file_path: &Path, | |
| opts: &ToolVersionOptions, | |
| ) -> Result<String> { | |
| let checksum = hash::file_hash_blake3(file_path, None)?; | |
| // Include extraction options in cache key to handle different extraction needs | |
| let mut cache_key_parts = vec![checksum.clone()]; | |
| if let Some(strip_components) = opts.get("strip_components") { | |
| cache_key_parts.push(format!("strip_{strip_components}")); | |
| } | |
| let cache_key = cache_key_parts.join("_"); | |
| debug!("Using file-based checksum as cache key: {}", cache_key); | |
| Ok(cache_key) |
### 🚀 Features - **(http)** Add 'format' to http backend by @thejcannon in [#6957](#6957) ### 🐛 Bug Fixes - **(bootstrap)** wrong directory on first run by @vmeurisse in [#6971](#6971) - **(tasks)** fix nested colons with `mise task edit` by @jdx in [#6978](#6978) - Use compatible env flags by @thejcannon in [#6964](#6964) - Flush vfox download buffer by @blampe in [#6969](#6969) ### 📚 Documentation - `arch()` template is `x64` by @thejcannon in [#6967](#6967) - update section headers in getting-started.md by @JunichiroKohari in [#6980](#6980) ### New Contributors - @JunichiroKohari made their first contribution in [#6980](#6980) - @blampe made their first contribution in [#6969](#6969) - @thejcannon made their first contribution in [#6964](#6964) ## 📦 Aqua Registry Updates #### New Packages (1) - [`cirruslabs/cirrus-cli`](https://github.com/cirruslabs/cirrus-cli) #### Updated Packages (1) - [`axodotdev/cargo-dist`](https://github.com/axodotdev/cargo-dist)
|
@jdx what do you think of that Cursor Bugbot comment? I actually think it may have legs... |
|
it might, I'm not sure |
(From discussion #6951)
Summary
This PR adds a
formatconfig tohttpbackend, to help in corner-cases where the format isn't given (or is wrong).Testing
Added a new E2E test (those are awesome!!)
Note: We could add more tests:
format = "gz"if you wanna upload more binaries!
Note
Adds a
formatoption (with platform-specific overrides) to force archive type detection when URLs lack or misstate extensions, with docs and e2e tests.format(incl. platform-specific) insrc/backend/http.rsby appending the given format to the downloaded file path for correct archive detection.formatoption and platform-specific formats indocs/dev-tools/backends/http.md.e2e/backend/test_http_formatcovering explicitformatand per-platformformat.Written by Cursor Bugbot for commit 3c978d7. This will update automatically on new commits. Configure here.