Skip to content

std: restructure child process namespace - #20049

Merged
andrewrk merged 2 commits into
masterfrom
std.process.Child
May 26, 2024
Merged

std: restructure child process namespace#20049
andrewrk merged 2 commits into
masterfrom
std.process.Child

Conversation

@andrewrk

@andrewrkandrewrk commented May 23, 2024

Copy link
Copy Markdown
Member

Upgrade guide:

-std.ChildProcess+std.process.Child

Some other functions are also moved from std.ChildProcess to std.process namespace.

In the future there will be even more breaking changes. For example, instead of creating a Child and then setting fields on it and then calling spawn, there will be std.process.spawn which takes an "options" parameter and then returns the Child, which is an object that lasts only from spawn until termination. This is a practice that we have been moving more towards in Zig, which is to have types designed to have minimal lifetimes and minimal states with undefined fields.

@andrewrkandrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. labels May 23, 2024
@andrewrk
andrewrk requested a review from squeek502May 23, 2024 18:34
@andrewrkandrewrk added the release notes This PR should be mentioned in the release notes. label May 23, 2024
@kubkon

Copy link
Copy Markdown
Member

Exciting!

@squeek502squeek502 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a commit that improves the doc comments of some functions since they are now actually exposed (they were pub before but not exposed from any standard library namespace).

I think this is fine as-is, but want to note that while argvToCommandLineWindows/argvToScriptCommandLineWindows/WindowsCommandLineCache are technically usable outside the context of process.Child, not too much thought went into their API for that purpose. IMO it would be equally valid to stick them in process.Child and make them non-pub, but I don't have strong feelings one way or the other.

@squeek502
squeek502force-pushed the std.process.Child branch 2 times, most recently from 76af53b to 1842b6dCompareMay 23, 2024 21:13
@andrewrk

Copy link
Copy Markdown
MemberAuthor

Ah, I didn't realize they were not actually exposed. In that case, I would rather move them into Child.zig and make them private. Is that okay with you?

@squeek502

squeek502 commented May 23, 2024

Copy link
Copy Markdown
Member

Is that okay with you?

Yep, definitely. There are quite a few nuances with them vis-a-vis BatBadBut.

You can leave createWindowsEnvBlock and createNullDelimitedEnvMap in std.process if you'd like, those are unrelated to the Windows argv stuff and likely more generally useful.

@andrewrk

andrewrk commented May 26, 2024

Copy link
Copy Markdown
MemberAuthor

Crap. I accidentally clobbered your commits @squeek502 - would you mind adding them back?

Edit: never mind, I still have them in my other zig checkout :-)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breakingImplementing this issue could cause existing code to no longer compile or have different behavior.release notesThis PR should be mentioned in the release notes.standard libraryThis issue involves writing Zig code for the standard library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewrk@kubkon@squeek502