Repository navigation
fix(origin): accept drive-letter file URIs and run the suite on Windows - #5
Conversation
url.Parse made C: the host of git+file://C:/path, so every repository on a Windows machine was refused; the file transport now takes file:///C:/path (RFC 8089) and the file://C:/path spelling, and the local-path errors name the form for this machine. The e2e harness builds syspi.exe on Windows. CI adds windows-latest to the gate with hg and svn from chocolatey, and .gitattributes keeps LF so gofmt holds there. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
…s-safe A Windows git with core.autocrlf=true rewrote every line ending in the checkout, so the tree digest and the .meta.json checksums no longer matched the other VCSs; syspi now clones with core.autocrlf=false. svn file URLs carry the drive as file:///C:/path. Test fixtures: svn URLs through fileURL, no ':' in directory names, the credential helper path in slash form, the lock-source rewrite escapes backslashes once, and the read-only-directory rollback test skips on Windows. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ead of prompting On Windows hg takes the NUL stdin for a terminal, prompts for the user name and dies with "response expected", which hid the authorization hint. The svn netrc test derives the repository path from the fixture root rather than the file URL. Co-Authored-By: jason.han <hanhuijun@gmail.com>
SlikSvn 1.14 on Windows takes a bare LF as part of the password (CRLF and no newline both work); svn on Linux and macOS reads to EOF, so the newline goes. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if drive.MatchString(rest[1:]) { | ||
| return rest[1:], true |
There was a problem hiding this comment.
🟡 Absolute local paths become relative
On POSIX, file:///C:/tools.kpar becomes C:/tools.kpar in fileURIPath. The archive loader resolves it under the project instead of /C:/tools.kpar, so an existing archive fails to install.
Learn more
A file URI with a first directory named C: is a valid absolute path on POSIX. The new drive check removes its leading slash without checking the host OS. resolveLocalArchive then joins the resulting relative path with the consuming project's root; parseVCSURL also removes the slash for repositories.
Example: On Linux, a real archive at /C:/tools.kpar is referenced as file:///C:/tools.kpar. Parsing yields C:/tools.kpar, so install looks for <project>/C:/tools.kpar and fails.
Recommended fix: Apply the drive normalization only on Windows in both fileURIPath and parseVCSURL; leave POSIX paths starting /C:/ absolute. Add a POSIX regression test for both local files and VCS sources.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed: on POSIX a directory named C: is legal and fileURIPath dropped its leading slash. #6 makes the drive check Windows-only (isDrivePath = windowsPaths && drive.MatchString) and adds TestDriveLetterFileURIs, which exercises both machines from one test file (file:///C:/tools.kpar → /C:/tools.kpar on POSIX, file://C:/x refused there).
| if runtime.GOOS == "windows" { | ||
| t.Skip("a read-only directory does not refuse new files on Windows") | ||
| } |
There was a problem hiding this comment.
🟡 Windows rollback loses its test
On Windows, TestPublishRollsBackWhenTheLockCannotBeWritten skips instead of testing rollback. The Windows gate cannot catch a failed lock write leaving lib/ inconsistent with the lock.
Learn more
The rollback test simulates a lock write failure by changing directory permissions. Windows does not enforce that permission change in the same way, so the new skip removes rollback coverage precisely on the newly gated platform. Run publishes packages before writing the lock and invokes rollback if writing fails.
Example: On Windows, a regression in lock-write failure handling can leave lib/ containing version 1.1.0 while the on-disk lock still names 1.0.0; the CI test exits as skipped.
Recommended fix: Replace the permissions-based failure setup on Windows with a deterministic lock-write failure that still lets lib/ be updated, then assert the old lock and installed tree remain intact. Retain the existing POSIX setup if needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed. #6 removes the skip: Windows ignores a read-only directory but refuses to replace a read-only file, so there the test makes .project.lock.json itself 0444; lock.Write's final os.Rename then fails after lib/ was published and the same rollback path is asserted (message "Access is denied" instead of "permission denied"). The Windows gate on #6 is the proof.
| if transport == "file" { | ||
| // file:///C:/path is the RFC 8089 form; file://C:/path is accepted as input. | ||
| if drive.MatchString(rest) { | ||
| rest = "/" + rest | ||
| } |
There was a problem hiding this comment.
🟡 Windows file URI contract remains undocumented
file://C:/path becomes accepted input, but the documented resource grammar lacks the Windows forms. CONTRIBUTING.md requires documentation updates when a format or contract changes.
Learn more
Example: A Windows user reading the resource grammar finds <vcs>+file:///… but no explanation of the drive-letter spelling accepted by the parser.
Recommended fix: Document the accepted Windows file:///C:/path and file://C:/path forms and their canonical identity in README.md or docs/packages.md, including plain local file:// resources.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Done in #6: docs/packages.md and the README now name <vcs>+file:///C:/path as the canonical Windows form, <vcs>+file://C:/path as accepted input normalised to it, the same two forms for a local .kpar or project directory, and that /C:/path is an ordinary directory on other systems.
Summary
Running the Go gate on
windows-latest(first done on #4) showed the suite had never run on Windows; this makes it pass there and adds Windows to the CI gate so it stays that way. #4 (release line) ships a Windows binary and depends on this.internal/origin:url.Parse("file://C:/Users/…")makesC:the host, so every repository on a Windows machine was refused with "a repository on this machine is git+file:///absolute/path". The file transport now acceptsgit+file:///C:/path(RFC 8089) and thegit+file://C:/pathspelling, storesPathasC:/path, and the local-path errors name the form for this machine (FileURIPath):file://resources (.kpar, project directories).tests/e2e: the harness buildssyspi.exeon Windows (execrefused the suffix-less file).ci.yml:windows-latestjoins the gate matrix with hg and svn from chocolatey (hg,sliksvn) onGITHUB_PATH, soSYSPI_REQUIRE_VCS=1holds on all three systems; gate steps run under bash on every runner..gitattributespins LF: a Windows checkout turned every file CRLF andgofmt -l .listed the whole tree.The Windows-only expectations in
origin_test.goare derived fromfilepath.Abs, so the test file is one source for all three systems.Verified: full gate on Linux (
go build,go vet,gofmt -l,go test ./...,SYSPI_REQUIRE_VCS=1) green; the Windows job on this PR is the first real Windows run, so further runner-specific findings may follow in this PR.