Skip to content

OS resources implementations too generalized, split into os specific directories #1239

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

Closed
bjornpagen opened this issue Jul 15, 2018 · 3 comments
Milestone

Comments

@bjornpagen
Copy link

bjornpagen commented Jul 15, 2018

Take for example std.os.child_process:

Exhibit A: std/os/child_process.zig:18,21

    pub const ChildProcess = struct {
    pub pid: if (is_windows) void else i32,
    pub handle: if (is_windows) windows.HANDLE else void,
    pub thread_handle: if (is_windows) windows.HANDLE else void,

Exhibit B: std/os/child_process.zig:134,176

    /// Forcibly terminates child process and then cleans up all resources.
    pub fn kill(self: *ChildProcess) !Term {
        if (is_windows) {
            return self.killWindows(1);
        } else {
            return self.killPosix();
        }
    }

    pub fn killWindows(self: *ChildProcess, exit_code: windows.UINT) !Term {
        if (self.term) |term| {
            self.cleanupStreams();
            return term;
        }

        if (!windows.TerminateProcess(self.handle, exit_code)) {
            const err = windows.GetLastError();
            return switch (err) {
                else => os.unexpectedErrorWindows(err),
            };
        }
        try self.waitUnwrappedWindows();
        return self.term.?;
    }

    pub fn killPosix(self: *ChildProcess) !Term {
        if (self.term) |term| {
            self.cleanupStreams();
            return term;
        }
        const ret = posix.kill(self.pid, posix.SIGTERM);
        const err = posix.getErrno(ret);
        if (err > 0) {
            return switch (err) {
                posix.EINVAL => unreachable,
                posix.EPERM => error.PermissionDenied,
                posix.ESRCH => error.ProcessNotFound,
                else => os.unexpectedErrorPosix(err),
            };
        }
        self.waitUnwrapped();
        return self.term.?;
    }

Many things in the Zig standard library are inherently unportable across operating systems, especially things in std.os, yet they share implementation in a common file. In this case, Windows and Posix both share their implementations in the same file, std.os.child_process.zig. This is in bad taste for several reasons:

  • because we have to call things like if (is_windows) too much
  • because we have to void out irrelevant variables (such as GID or PID on Windows)

Solution:
Do a platform based assignment in std/os/index.zig like so:

const std = @import("../index.zig");
const os = std.os;
const windows = os.windows;
const linux = os.linux;                 // note: this does not exist yet, but it should. info more far down
const macos = os.macos;           // note: this does not exist yet, but it should. info more far down
pub const ChildProcess = switch(builtin.os) {
    bulitin.Os.windows => windows.child_process,
    builtin.Os.linux => linux.child_process,
    builtin.Os.macos => macos.child_process,
};
// other stuff

Implement the actual thing in the os-specific directory.
Linux and macos now are to provide their own implementations, and not rely on POSIX. There may be codesharing between POSIX OS's, but they should rewrite it explicitly anyway. Implementation of std.os.linux.child_process:

const std = @import("../index.zig");
const os = std.os
// whatever other imports we need
pub const ChildProcess = struct {
   // linux specific spawn, we use clone() instead of fork() as an example (maybe it's faster idk)
    pub fn spawn() {
        //local implemtation goes here...
    }
    // other posix stuff
};

Much cleaner to read, and much easier to add a new OS implementation.

@bjornpagen
Copy link
Author

bjornpagen commented Jul 15, 2018

EDIT: Obviously this needs more discussion before I feel good about it, so rip this proposal to shreds. :)
EDIT2: Also, this means every version of std.os must expose exactly the same functions and values, otherwise this will not work. But since std.os only implements portable interfaces, this should not be an issue.

@andrewrk
Copy link
Member

andrewrk commented Jul 15, 2018

I don't agree with the problem statement:

because we have to call things like if (is_windows) too much

Putting the alternate code in another file doesn't make it simpler. We're either going to have these if statements, or duplicated code.

Have a look at std.event.Loop and see what you think. It's a great example of complex code that works on Linux, Windows, and MacOS, but with the same basic structure.

because we have to void out irrelevant variables (such as GID or PID on Windows)

I would argue this makes the implementation cleaner because it shows what exactly is different and what is the same between different operating systems.

Note that we currently do have std.os.linux and std.os.windows and they are the syscall functions. (For Windows it's the kernel32.dll functions).

@andrewrk andrewrk added this to the 0.3.0 milestone Jul 15, 2018
@bjornpagen
Copy link
Author

Convinced. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants