Support file:// protocol for BAZELISK_BASE_URL#753
Support file:// protocol for BAZELISK_BASE_URL#753hehaoqian wants to merge 3 commits intobazelbuild:masterfrom
Conversation
hehaoqian
force-pushed
the
master
branch
4 times, most recently
from
fe4a1dc to
f0a0dd4
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the file:// protocol in BAZELISK_BASE_URL to enable storing and accessing multiple Bazel versions from local disk without requiring an HTTP server. This is particularly useful for Docker images with pre-downloaded Bazel versions in network-restricted environments.
Changes:
- Added
readLocalFilefunction to handlefile://URLs by reading files from local disk - Introduced
LocalFileErrortype to distinguish local file errors from network errors for retry logic - Modified
getfunction to detect and handlefile://URLs before attempting HTTP requests - Updated
shouldRetryfunction to prevent retries for local file errors - Added test coverage for reading local files and handling missing file errors
- Updated README with documentation on using
file://URLs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| httputil/httputil.go | Implements file:// URL support with local file reading, error handling, and retry logic |
| httputil/httputil_test.go | Adds tests for successful local file reads and error handling for missing files |
| README.md | Documents the file:// URL feature for local Bazel version storage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := os.WriteFile(path, []byte(want), 0644); err != nil { | ||
| t.Fatalf("failed to write temp file: %v", err) | ||
| } | ||
| fileURL := (&url.URL{Scheme: "file", Path: path}).String() |
There was a problem hiding this comment.
The test uses url.URL construction to create file URLs, which handles platform-specific differences correctly. However, the actual implementation in readLocalFile doesn't parse URLs the same way. This could cause the test to pass on Unix-like systems but fail on Windows. Consider adding a Windows-specific test case or ensuring the implementation matches the URL construction approach used in tests.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| var nre *LocalFileError | ||
| if errors.As(err, &nre) { |
There was a problem hiding this comment.
The variable name nre is unclear and doesn't clearly indicate it represents a LocalFileError. Consider using a more descriptive name like localFileErr or fileErr to improve code readability.
| var nre *LocalFileError | |
| if errors.As(err, &nre) { | |
| var localFileErr *LocalFileError | |
| if errors.As(err, &localFileErr) { |
| func readLocalFile(urlStr string) (*http.Response, error) { | ||
| urlStr = strings.TrimPrefix(urlStr, "file://") | ||
| path, err := url.PathUnescape(urlStr) | ||
| if err != nil { | ||
| return nil, &LocalFileError{err: fmt.Errorf("invalid file url %q: %w", urlStr, err)} | ||
| } | ||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return nil, &LocalFileError{err: fmt.Errorf("could not open %q: %w", path, err)} | ||
| } |
There was a problem hiding this comment.
The function doesn't handle edge cases for malformed file URLs. For example, if the URL is just file:// (with no path), the code will attempt to open an empty path which could lead to confusing error messages. Consider adding validation to check for empty or invalid paths after URL parsing.
| @@ -78,7 +79,41 @@ func ReadRemoteFile(url string, auth string) ([]byte, http.Header, error) { | |||
| return body, res.Header, nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The LocalFileError type lacks a documentation comment. According to Go conventions, exported types should have documentation comments. Consider adding a comment like: LocalFileError wraps errors that occur when reading local files via file:// URLs, allowing them to be distinguished from network errors for retry logic.
| // LocalFileError wraps errors that occur when reading local files via file:// URLs, | |
| // allowing them to be distinguished from network errors for retry logic. |
| func readLocalFile(urlStr string) (*http.Response, error) { | ||
| urlStr = strings.TrimPrefix(urlStr, "file://") | ||
| path, err := url.PathUnescape(urlStr) | ||
| if err != nil { | ||
| return nil, &LocalFileError{err: fmt.Errorf("invalid file url %q: %w", urlStr, err)} | ||
| } |
There was a problem hiding this comment.
The file URL parsing implementation has cross-platform issues. On Windows, file URLs use the format file:///C:/path/to/file (note three slashes). After stripping file://, this leaves /C:/path/to/file, which is not a valid Windows path. The code should use url.Parse to properly extract the path component, which automatically handles platform-specific conversions. For example, url.Parse("file:///C:/path/file").Path would return /C:/path/file on Unix but C:/path/file on Windows when using proper URL parsing libraries. Consider using url.Parse(urlStr) followed by accessing parsed.Path to get the correct file path.
|
|
||
| You can also override the URL by setting the environment variable `$BAZELISK_BASE_URL`. Bazelisk will then append `/<VERSION>/<FILENAME>` to the base URL instead of using the official release server. Bazelisk will read file [`~/.netrc`](https://everything.curl.dev/usingcurl/netrc) for credentials for Basic authentication. | ||
|
|
||
| If you want to use the releases stored on the local disk, set the URL as `file://` followed by the local disk path. On Windows, escape `\` in the path by `%5C`. |
There was a problem hiding this comment.
The README documentation suggests using %5C to escape backslashes in Windows paths, but this is misleading. The proper way to specify Windows paths in file URLs is to use forward slashes, like file:///C:/path/to/folder. URL-encoding backslashes as %5C would result in the literal string containing backslashes after URL decoding, which may not work correctly. Consider revising the documentation to recommend using forward slashes in file URLs for all platforms, including Windows, as this is the standard approach for file URLs.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
1 participant
Fixes #715
In order to make it convenience to store multiple bazel versions in local disk directly,
instead of setup a HTTP server for it.
Especially useful for a docker image of multi bazel version, in an network restricted environment.