Use x/sys/windows instead of syscall #197
Conversation
| ) | ||
|
|
||
| //sys bind(s syscall.Handle, name unsafe.Pointer, namelen int32) (err error) [failretval==socketError] = ws2_32.bind | ||
| //sys bind(s windows.Handle, name unsafe.Pointer, namelen int32) (err error) [failretval==socketError] = ws2_32.bind |
There was a problem hiding this comment.
I was super-disappointed that windows.Sockaddr is not implementable outside x/sys/windows. On the other hand, it has a buffer that's only 116 bytes, so we couldn't fit a (Edit: I can't count, a GUID is 128 bits, not 128 bytes). I guess it's a future problem to add HvsockAddr in there anyway right nowAF_HYPERV and SockAddrHyperV to x/sys/windows directly.
|
I think #172 is also still pending (moving some changes to upstream golang.org/x/sys) |
|
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 I have replaced |
|
@kevpar @containerplat PTAL |
|
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 |
|
It wouldn't be an issue if we want to keep the vhd package's API using 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 |
|
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. |
|
If we want to "rip off" the bandaid, should we also include #172 in the same release? (perhaps that one needs to be carried as it looks like it needs a rebase) |
|
@thaJeztah That sounds ideal. Paging @katiewasnothere |
|
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 If things go really bad (some important LTS project using |
|
Yeah, I'm almost certain that at no point has Go let me silently replace 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...) |
|
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. |
|
wdyt @kevpar @dcantah @katiewasnothere ? |
|
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
force-pushed
the
use-x_sys_windows-instead-of-syscall
branch
2 times, most recently
from
0ba5488
to
71b2a58
Compare
I'd like to highlight a versioning issue we hit at moby/moby#42307 (comment), 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". |
Yes, I saw the failure, and 🤦 realised that those changes were already in master before the last v0.4 tag was done |
This reverts commit bfd5468. Signed-off-by: Kathryn Baldauf <[email protected]>
This reverts commit ef753e6. Signed-off-by: Kathryn Baldauf <[email protected]>
Signed-off-by: Kathryn Baldauf <[email protected]>
Add CI github action for testing on push and PR
Signed-off-by: Kathryn Baldauf <[email protected]>
Add build status badge to readme
Even though this repo is normally used as a dependency, there are some sample binaries and tools built out of here. This just makes sure they still build in the CI. Signed-off-by: Daniel Canter <[email protected]>
Build the three binaries in this repo from the ci
…_break [Temporary] Revert Implement winio.GetFileStandardInfo FileInfo commits
…einfo_break Revert removal of "Implement winio.GetFileStandardInfo FileInfo" commits
Specifically, this pulls the fix for golang/go#44538 as we're going to start using this API in place of our own generated wrapper. Signed-off-by: Paul "TBBle" Hampson <[email protected]>
This replaces the use of either syscall or hard-coded magic numbers. Signed-off-by: Paul "TBBle" Hampson <[email protected]>
syscall.Handle is core to a _lot_ of structures and APIs, so this great-big commit contains the entire rest of the conversion from syscall to x/sys/windows. I also eliminated a bunch of generated syscall wrappers which had _direct_ equivalents in x/sys/windows. There's still a bunch which exist in x/sys/windows and either have worse implementations, e.g. windows.CreateFile, or somehow expose different APIs, e.g. the calls used by privileges.go Signed-off-by: Paul "TBBle" Hampson <[email protected]>
This basically happened by accident while I was working on #185. Pretty-much everything used from
syscallalso appears inx/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
syscallare the syscall wrappers generated bygolang.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".