windows: restore core indexing and search support - #941
Conversation
AI PromptIn commit 70647ba and 0d03621 we reverted support for this project building and working on windows. The project has changed quite a bit since, can you re-add support? You can run Note this for why we reverted support for windows initially: So on most platforms can we stick to hardcoding the page size. |
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
Thanks for tackling this and reverting my revert :)
| ConstLabels: prometheus.Labels{"path": path}, | ||
| }, func() float64 { | ||
| usage, _ := disk.Usage(path) | ||
| // I know there is no error handling here, and I don't like it |
There was a problem hiding this comment.
Tiny comment: these comments are unrelated to the change and not super useful (and also use "I" in an inaccurate way :))
| // limitations under the License. | ||
|
|
||
| //go:build linux || darwin | ||
| //go:build !windows |
There was a problem hiding this comment.
Small comment, this file could be named indexfile_unix.go for consistency with the others.
| } | ||
|
|
||
| func (f *indexFileFromOS) Read(off, sz uint32) ([]byte, error) { | ||
| r := make([]byte, sz) |
There was a problem hiding this comment.
If we are really supporting Windows, we should use mmap here instead of direct reads. This provides the best experience, and I think it's an easier mental model for debugging/ support, to consistently use file mapping. Maybe we could re-introduce mmap-go, but just here for Windows? Or implement it directly, doesn't look too bad.
|
|
||
| package index | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
This part doesn't look right to me -- isn't this already handled in builder_windows.go?
By the way, I'm generally confused on why we are storing the system umask and manually applying it sometimes :)
|
@jtibshirani thanks for the review! I haven't even read this yet, this was totally vibe coding (but of course would be reviewed and improved by me before marking ready for review). |
|
hehe understood! I was just too interested and wanted to get in a review this week :) |
|
Thank you for fixing this issue. However, I'm currently avoiding building zoekt for Windows in Gitea by using the go build tag "!unix" as a temporary workaround. I'll only re-enable this capability once you restore Windows support. Unfortunately, I don't have a Windows environment to test zoekt either:( |
36f974a to
e37a1a3
Compare
keegancsmith
changed the title
d55fe4a to
dec5130
Compare
Restore the Windows webserver and Git indexer without changing the Unix mmap behavior that motivated the earlier revert. Keep mmap-go isolated to Windows, make repository paths platform-independent, and run full index tests plus focused Git-path coverage on Windows so real portability regressions are caught without lengthening the critical CI path. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a06b83-5fdf-72ff-9a07-614d9a2b5f97
dec5130 to
d891378
Compare
|
Follow-up from the Windows CI investigation:
All CI is green. The full index suite took 49s versus 47s for the previous targeted test, so broader coverage adds negligible test time. The 2m45s total Windows job was primarily caused by a variable 1m28s Go cache/setup step. |
3 participants
Restores Windows support for
zoekt-git-indexandzoekt-webserverwhile leaving the Unix mmap implementation unchanged. Windows index reads usemmap-gobehind Windows build constraints; Linux metrics remain Linux-only. Repository paths and globs now use consistent/semantics on every OS, and fixture inputs remain LF-stable.Windows CI runs the full index suite, a focused Git-path test, and builds both supported binaries. Literal
go test ./...remains unavailable on Windows becausezoekt-local-syncdepends on Unix file locking andzoekt-sourcegraph-indexserverdepends on Unix signals andumask; those commands remain out of scope.