Add forked version of os related stdlib packages #1134
Conversation
Due to some functionality in the stdlib not being supported and not wanting to reimplement os/exec.Cmd and friends, we need to fork the stdlib. The functionality being mentioned is related to not being able to do some setup with STARTUPINFOEXW and exposing it in a go friendly way in the stdlib isn't really feasible as there's currently no way to create a job object in the stdlib, and the syscall package is locked down. Signed-off-by: Daniel Canter <dcanter@microsoft.com>
This change edits a whole bunch of stuff in the stdlib fork to get things actually building. This includes things such as removing the imports to /internal packages in the stdlib (as we can't access them), changing around the fields of some of the calls to point to our forked version instead of the stdlib, and changing a whole slew of syscall definitions to x/sys/windows equivalents. Also satisfying golangci-lint by ignoring some errors and removing dead code that isn't used. Additionally adds a small test to make sure it works. Signed-off-by: Daniel Canter <dcanter@microsoft.com>
| } | ||
|
|
||
| var zeroProcAttr ProcAttr | ||
| var zeroSysProcAttr SysProcAttr | ||
|
|
||
| func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle uintptr, err error) { | ||
| if len(argv0) == 0 { | ||
| return 0, 0, EWINDOWS | ||
| return 0, 0, syscall.EINVAL |
There was a problem hiding this comment.
we don't want EWINDOWS here?
There was a problem hiding this comment.
We do, don't know how this got swapped.
| @@ -263,10 +267,10 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle | |||
| } | |||
|
|
|||
| if len(attr.Files) > 3 { | |||
| return 0, 0, EWINDOWS | |||
| return 0, 0, syscall.EINVAL | |||
There was a problem hiding this comment.
same question about EWINDOWS
There was a problem hiding this comment.
I'm not sure how these got swapped.. but good catch
| @@ -25,14 +25,14 @@ import ( | |||
| // will be sourced from syscall.Environ(). | |||
| func Default(sys *syscall.SysProcAttr) (env []string, err error) { | |||
| if sys == nil || sys.Token == 0 { | |||
| return syscall.Environ(), nil | |||
| return windows.Environ(), nil | |||
There was a problem hiding this comment.
Does this not build without replacing syscall with windows? Just wondering if we can avoid that so as to keep deviation from upstream as little as possible.
There was a problem hiding this comment.
No this would build fine, I swapped to Windows as there's a x/sys/Windows replacement/mirror for everything in syscall and technically the syscall package is locked and some of it is deprecated. I can swap to syscall if preferred. @anmaxvl What do you think?
There was a problem hiding this comment.
I misread the original code a bit, I thought it was internal/syscall and we replaced it with windows, turns out it was originally internal/syscall/windows and syscall was imported on its own.
I agree with @ambarve that we should probably keep syscall here.
There was a problem hiding this comment.
Will try and add everything thats touched in and update, I'll ping when done!
|
Going to add all of the internal/syscall/window definitions as suggested even though they basically exist as is in x/sys/windows just to keep this more similar in line with the stdlib. |
|
Closing in favor of this work #1233 Three tries, all for naught 😆 |
Try number three 😆
This change adds a forked version of some of the packages from the stdlib. It's currently split into two commits, one that brings in the stdlib packages we need as is, and another that edits them to get them actually functioning. This PR doesn't add any of the functionality described below, it simply gets the forked stdlib in a working state. Please see below for why this is needed/desired.
Why
Package stdlib is a fork of a small set of packages from the go stdlib and specifically the os, syscall, os/exec, and /internal/syscall/execenv packages. It exists because the process execution mechanism in the stdlib (os/exec.Cmd and everything in it's call chain all the way down to syscall.StartProcess) currently don't expose what's necessary to be able to accomplish some things that we need on Windows. This boils down to three things currently.
exec.Cmd.Start()callsexec.LookPathwhich looks for certain windows extensions to launch an executable at the path provided and will fail if one is not found. Although rare, there are cases of binaries with no extension that are perfectly valid and able to be launched byCreateProcessWthat will fail to launch with the default behavior. Granted this can be worked around by setting PATHEXT to contain an entry with just spaces after a colon separator, so this is more of an added annoyance for the other two reasons.For example:
For job containers we'd like the ability to launch a process in a job directly as it's created, instead of launching it and assigning it to the job slightly afterwards. This introduces a small window where the process is unaccounted for and not in the "container".
The desired behavior can be accomplished in Windows by calling UpdateProcThreadAtrribute and using the constant
PROC_THREAD_ATTRIBUTE_JOB_LIST. For the stdlib to support this it would need a new field off of syscall.SysProcAttr to beable to pass in the job object handle that you'd like the process added to. However, there is no way to create a job object in the stdlib itself and the syscall package is locked down for the most part.
Almost same story as 2, but in this case we'd like to support assigning a pseudo console to a process. There's no exposed way to pass in the pseudo console handle and there's no syscall package support for making one in the first place.
The stdlib packages have been modified to use the x/sys/windows package where needed, removed some unneccesary functionality that we don't need, as well as added the additions described above.