Use x/sys/windows instead of syscall

[TBBle]

This basically happened by accident while I was working on #185. Pretty-much everything used from syscall also appears in x/sys/windows, and a bunch of stuff in the latter isn't in the former, so this is a net deletion of LoC, which is nice.

The only things left that import syscall are the syscall wrappers generated by golang.org/x/sys/windows/mkwinsyscall.

I apologise in advance to reviewers for the second commit. it was one search-and-replace followed by fixing compiler errors, and that turned into "convert the world".

[thaJeztah]

I think https://github.com/microsoft/go-winio/pull/172 is also still pending (moving some changes to upstream golang.org/x/sys)

[TBBle]

Oh, interesting. With a brief look, I suspect there should not be a semantic conflict with #172, because that PR didn't change any of the syscall-sourced types, and I didn't try and replace existing code with x/sys/windows where the types differ, e.g. []byte => *windows.SECURITY_DESCRIPTOR, just the ones where I could literally replace syscall with windows and find the right thing.

I have replaced localFree with windows.LocalFree though, as has #172 in a few places, and I think I've done a pass previously to get rid of the (*[0xffff]byte)(unsafe.Pointer(X))[:Xlen] idiom, since that fails a gc check added in go 1.14. (That latter change might have been in hcsshim, not go-winio though. I've lost track, it was biting me when vendor'd into containerd anyway).

[thaJeztah]

@kevpar @containerplat PTAL

[dcantah]

All of these look fine (and thank you for doing it) besides the vhd ones as most of the signatures change. When updating the vhd package recently I made an effort to keep using syscall to not pull the rug up from anyone. I think we'd be fine taking this and cutting a release that documents the vhd funcs take in and return the windows equivalents now, but someone chime in with any objections here

[TBBle]

It wouldn't be an issue if we want to keep the vhd package's API using syscall.Handle, it's fairly isolated, so it wasn't caught up in the win32File.handle change that triggered this whole adventure, and it doesn't have any relationships with things in syscall that are differently structured in x/sys/windows.

But the question then becomes "will it ever change" and if so, "when is best to do it?".

I'd lean towards "earlier", while the API is (AFAIK) not otherwise changing, so there's only one upgrade pain-point at a time. And maybe it'll serve as a prompt for the consumers to also migrate from syscall to x/sys/windows. ^_^

[dcantah]

I think if we cut a release after the change we should be fine. Agreed sooner rather than later, I hope the API for that package is pretty locked in by now.

[thaJeztah]

If we want to "rip off" the bandaid, should we also include https://github.com/microsoft/go-winio/pull/172 in the same release? (perhaps that one needs to be carried as it looks like it needs a rebase)

[dcantah]

@thaJeztah That sounds ideal. Paging @katiewasnothere

[thaJeztah]

As to "breaking" change; I (hope) the change doesn't go silently; i.e. better to fail hard than "silent"; at least that should bring it to user's attention that something changed.

Technically (assuming "SemVer(ish)"), this project is still "unstable" (pre-1.0), so "anything goes". That said, I would recommend releasing these changes as v0.5.0 (incrementing the "minor" version), and make sure to add a "Breaking change" note in the release notes. Doing so might trigger the curiosity of users ("hey, what's new?"), but more importantly, releasing as v0.5.0 would "reserves" v0.4.x versions for future use.

If things go really bad (some important LTS project using v0.4.x, and there's a critical bug in go-winio), it would still provide the opportunity to do a v0.4.17 release (preventing such project from having to include the breaking change to get the important bugfix). Of course, ideally, such a v0.4.x release would not be needed, but just in case, it probably won't hurt to have it available as an option.

[TBBle]

Yeah, I'm almost certain that at no point has Go let me silently replace syscall.Handle with windows.Handle, despite both being aliases for the same underlying type (i.e. it would be safe if this was silent), so this won't be a silent change for users of the VHD package.

Coming from a C++ background, that's a pleasant surprise, much as it lead to, well, this PR (since I changed one structure, and just kept fixing compile errors...)

[TBBle]

As an update, I was expecting to land this after #172, which I thought was ready-to-go in March, but seems to have stalled. I expect if this lands first, #172 shouldn't need too much reworking, if we want to get this moving.

I also don't mind if this just waits for #172, so we can put them in together (batching the API changes), this one isn't blocking me at this time.

[thaJeztah]

v0.4.18 was tagged (https://github.com/microsoft/go-winio/pull/197#issuecomment-795676667), so I think it should now be ok to merge the breaking changes (targeting v0.5.x)

wdyt @kevpar @dcantah @katiewasnothere ?

[dcantah]

The plan was to have one final .4.x tagged release (0.4.17) before this got in and then bump to 0.5.x with this breaking change, but we had one extra requested addition for Moby the other day so we cut another release. This looks fine to merge to me. @katiewasnothere was there anything extra needed for the SecurityAttribute change? Otherwise let's get both of these in and cut a new release.

[TBBle]

The plan was to have one final .4.x tagged release (0.4.17) before this got in and then bump to 0.5.x with this breaking change, but we had one extra requested addition for Moby the other day so we cut another release.

I'd like to highlight a versioning issue we hit at https://github.com/moby/moby/pull/42307#issuecomment-823661611, and that we might want to undo an API change from #185, release an 0.4.19, and then redo the API change and merge this and #172.

That's not the only feasible solution to that issue, but I wanted to make sure the discussion was visible, since this PR is "continue/complete the ABI change I introduced in-passing in #185".

[thaJeztah]

I'd like to highlight a versioning issue we hit at moby/moby#42307 (comment)

Yes, I saw the failure, and 🤦 realised that those changes were already in master before the last v0.4 tag was done