-
Notifications
You must be signed in to change notification settings - Fork 405
Support file:// protocol for BAZELISK_BASE_URL #753
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ import ( | |||||||||
| "path/filepath" | ||||||||||
| "regexp" | ||||||||||
| "strconv" | ||||||||||
| "strings" | ||||||||||
| "time" | ||||||||||
|
|
||||||||||
| netrc "github.com/bgentry/go-netrc/netrc" | ||||||||||
|
|
@@ -78,7 +79,41 @@ func ReadRemoteFile(url string, auth string) ([]byte, http.Header, error) { | |||||||||
| return body, res.Header, nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
||||||||||
| // LocalFileError wraps errors that occur when reading local files via file:// URLs, | |
| // allowing them to be distinguished from network errors for retry logic. |
Copilot
AI
Jan 31, 2026
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 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.
Copilot
AI
Jan 31, 2026
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 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.
Copilot
AI
Jan 31, 2026
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 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) { |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,9 @@ package httputil | |
| import ( | ||
| "errors" | ||
| "net/http" | ||
| "net/url" | ||
| "os" | ||
| "path/filepath" | ||
| "strconv" | ||
| "strings" | ||
| "testing" | ||
|
|
@@ -251,3 +254,42 @@ func TestNoRetryOnPermanentError(t *testing.T) { | |
| t.Fatalf("Expected no retries for permanent error, but got %d", clock.TimesSlept()) | ||
| } | ||
| } | ||
|
|
||
| func TestReadLocalFile(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| path := filepath.Join(tmpDir, "payload.txt") | ||
| want := "hello from disk" | ||
| 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() | ||
|
||
|
|
||
| body, _, err := ReadRemoteFile(fileURL, "") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got := string(body); got != want { | ||
| t.Fatalf("expected body %q, but got %q", want, got) | ||
| } | ||
| } | ||
|
|
||
| func TestReadLocalFileNotFound(t *testing.T) { | ||
| clock := newFakeClock() | ||
| RetryClock = clock | ||
| MaxRetries = 10 | ||
| MaxRequestDuration = time.Hour | ||
|
|
||
| missingPath := filepath.Join(t.TempDir(), "does-not-exist.txt") | ||
| fileURL := (&url.URL{Scheme: "file", Path: missingPath}).String() | ||
|
|
||
| _, _, err := ReadRemoteFile(fileURL, "") | ||
| if err == nil { | ||
| t.Fatal("expected error for missing file") | ||
| } | ||
| if !errors.Is(err, os.ErrNotExist) { | ||
| t.Fatalf("expected os.ErrNotExist, got %v", err) | ||
| } | ||
| if clock.TimesSlept() != 0 { | ||
| t.Fatalf("expected no retries for file:// error, but slept %d times", clock.TimesSlept()) | ||
| } | ||
| } | ||
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 README documentation suggests using
%5Cto escape backslashes in Windows paths, but this is misleading. The proper way to specify Windows paths in file URLs is to use forward slashes, likefile:///C:/path/to/folder. URL-encoding backslashes as%5Cwould 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.