all: re-implement Windows support #941
Conversation
This commit re-adds support for building and running Zoekt on Windows platforms. We had previously reverted Windows support in commits 70647ba and 0d03621 due to possible memory mapping issues affecting Linux servers. The key changes are: - Added platform-specific implementations for file handling - Re-introduced Windows-specific signal handling - Fixed disk usage reporting on Windows via filepath.VolumeName - Used OS-specific build tags to keep the page size hardcoded on non-Windows platforms. We maintain the approach of using a hardcoded page size (4KB) on non-Windows platforms, which was the key reason for the initial support removal, while providing correct Windows implementations. Test Plan: Successfully builds with GOOS=windows GOARCH=amd64
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. |
| func mustRegisterDiskMonitor(path string) { | ||
| prometheus.MustRegister(prometheus.NewGaugeFunc(prometheus.GaugeOpts{ | ||
| Name: "src_disk_space_available_bytes", | ||
| Help: "Amount of free space disk space.", | ||
| 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 :))
| @@ -12,7 +12,7 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // 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:( |
This commit re-adds support for building and running Zoekt on Windows platforms. We had previously reverted Windows support in commits 70647ba and 0d03621 due to possible memory mapping issues affecting Linux servers.
The key changes are:
We maintain the approach of using a hardcoded page size (4KB) on non-Windows platforms, which was the key reason for the initial support removal, while providing correct Windows implementations.
Test Plan: Successfully builds with GOOS=windows GOARCH=amd64