testing: flesh out winfsp, add to CI, expand CI, fix gitstore bug #16
Conversation
a0d99fe to
c865c2f
Compare
25c5efa to
3f9a4cc
Compare
b8a149e to
7ac7182
Compare
3ff04e1 to
f4a1e44
Compare
f4a1e44 to
9125e67
Compare
There was a problem hiding this comment.
Not really relevant to this PR, but as a general comment it might be nice to create a wrapper function for CombinedOutput that includes the output in the error if it's non-nil. I had an old version of git causing local tests to fail, and to get any details I had to go find the right call site and update the error by hand (it's just e.g. "exit status 129").
Not a big deal on its own, but I can imagine it will be hard to debug prod issues for the same reason if we ever encounter unexpected issues with git.
| // from OpenFile calls. Otherwise PowerShell sets it and then | ||
| // winfsp/gofs rejects the open, which is arguably a bug. |
There was a problem hiding this comment.
I have to admit I am not 100% following this sequence of events. Is PowerShell involved because we're running a build from PowerShell, and the issue is you can't have 2 different processes open the same file with this bit set?
There was a problem hiding this comment.
No, even interactive use from PowerShell and doing "type .gomod-status" was failing because PowerShell sets that bit.
I'm working around a ~bug in our fsp Go bindings which are arguably too defensive/paranoid.
But not working in PowerShell is then also annoying for GitHub Actions for steps that want to use a shell.
| // the "base" filesystem returned by [gofs.New]. | ||
| type goFSImpl interface { | ||
| winfsp.BehaviourBase | ||
| winfsp.BehaviourGetSecurityByName |
There was a problem hiding this comment.
It's not immediately obvious why we include all these interfaces as well as the base one that is accepted by the gofs function signature. AFAIU gofs.New optionally switches behaviour on if the type implements these?
There was a problem hiding this comment.
gofs.New should really be returning an exported concrete type so it can be embedded without losing optional methods.
But gofs returns only winfsp.BehaviourBase even though it implements a dozen more methods. If we used its static type as our embedded type, we'd then lose the "forwarding" of those dozen methods and the fsp.Mount wouldn't call those methods and we'd fail to work (as I discovered).
So I had to forward all of these methods too.
I want to send a PR to upstream to change that signature.
This runs "go mod verify" against gomodfs in NFS mode on all three OSes (linux, darwin, windows) and additionally runs Windows in WinFsp mode. In the process, this also implements the magic ".gomodfs-status" file work in PowerShell with WinFsp. (Powershell sets FILE_OPEN_NO_RECALL which go-winfsp was rejecting. So this adds a wrapper that unsets that bit.) And this fixes a gitstore bug that made "go mod verify" fail on a repo with exotic filenames. (#15) Updates tailscale/corp#34812 Fixes #15 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
9125e67 to
3cf4e70
Compare
This runs "go mod verify" against gomodfs in NFS mode on all three
OSes (linux, darwin, windows) and additionally runs Windows in WinFsp
mode.
In the process, this also implements the magic ".gomodfs-status" file
work in PowerShell with WinFsp. (Powershell sets FILE_OPEN_NO_RECALL
which go-winfsp was rejecting. So this adds a wrapper that unsets
that bit.)
And this fixes a gitstore bug that made "go mod verify" fail on a repo
with exotic filenames. (#15)
Updates tailscale/corp#34812
Fixes #15