-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to go-winio/tools/mkwinsyscall #1409
Conversation
@@ -7,7 +7,7 @@ import ( | |||
hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" | |||
) | |||
|
|||
//go:generate go run ../mksyscall_windows.go -output zsyscall_windows.go storage.go | |||
//go:generate go run github.com/Microsoft/go-winio/tools/mkwinsyscall -output zsyscall_windows.go storage.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt even know you could do a go run
from a remote. Thats crazy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you try and use $GOROOT/src/syscall/mksyscall_windows.go
, it tells you the prefered way golang.org/x/sys/windows/mkwinsyscall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also did not know..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this actually work for a vendor based project (one that had winio vendored)? I did a quick test of just vendoring x/sys/windows and doing the go:generate dance and specifying golang.org/x/sys/windows/mkwinsyscall, it complains with:
cannot find package "github.com/dcantah/generate/vendor/golang.org/x/sys/windows/mkwinsyscall" in:
C:\Users\danny\go\src\github.com\dcantah\generate\vendor\golang.org\x\sys\windows\mkwinsyscall
which is honestly what I thought would happen, but maybe there's more I'm missing. I don't think we'd ever actually have an import from the tools/mkwinsyscall pkg so mod vendor wouldn't pull in the files afaict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is a way to vendor the tool:
https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module
I have to wait until upstream go-winio gets updated, since it fails to vendor and update.
I am not sure if Go respects that local copy when building it though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a shot, it sounds like there is a way though
) | ||
|
||
// errnoErr returns common boxed Errno values, to prevent | ||
// allocations at runtime. | ||
func errnoErr(e syscall.Errno) error { | ||
switch e { | ||
case 0: | ||
return nil | ||
return errERROR_EINVAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. EINVAL is 22 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but errnoErr
is only called if the return condition is true. So if the return value is invalid, but the error number is 0 for some reason, then it returns EINVAL:
if handle == 0 {
err = errnoErr(e1)
}
The wclayer and winapi syscall files have examples of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take your word for it :). It doesnt make sense to me why if the syscall failed that Win32 would not give us the error that it wants and we would instead overwrite it with a value of EINVAL. When would Win32 ever return us S_OK when a function failed? But I digress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also am of the same opinion.
I can add a flag to disable that behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions
7793230
to
acfef7f
Compare
ba5f9c7
to
9afaf57
Compare
Added `.\tools.go" file to add mkwinsyscall to `go.mod` and track as a dependency, as recommended by the [go wiki](https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module). Using `-sort=false` flag to make changes easier to see. (Will be undone in next commit.) Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
9afaf57
to
486a9f1
Compare
* Switch to go-winio/tools/mkwinsyscall Added `.\tools.go" file to add mkwinsyscall to `go.mod` and track as a dependency, as recommended by the [go wiki](https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module). Using `-sort=false` flag to make changes easier to see. (Will be undone in next commit.) Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> * undoing sort=false Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
* Switch to go-winio/tools/mkwinsyscall Added `.\tools.go" file to add mkwinsyscall to `go.mod` and track as a dependency, as recommended by the [go wiki](https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module). Using `-sort=false` flag to make changes easier to see. (Will be undone in next commit.) Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> * undoing sort=false Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Switching to a single mkwinsyscall tool that is forked and updated for our needs instead of the different local copies in the repo.
Added
.\tools.go" file to add mkwinsyscall to
go.mod` and track as a dependency, as recommended by the go wiki.This has two commits: the first is of updates with
-sort=false
, so that changes are easier to check.The next re-enables sorting (the default behavior) and switches to using a glob pattern for
internal/winapi
.Relies on: microsoft/go-winio#248