Skip to content

std: restructure child process namespace #20049

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

Merged
merged 2 commits into from
May 26, 2024
Merged

std: restructure child process namespace #20049

merged 2 commits into from
May 26, 2024

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented May 23, 2024

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.

@andrewrk andrewrk 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 squeek502 May 23, 2024 18:34
@andrewrk andrewrk added the release notes This PR should be mentioned in the release notes. label May 23, 2024
@kubkon
Copy link
Member

kubkon commented May 23, 2024

Exciting!

Copy link
Collaborator

@squeek502 squeek502 left a comment

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 squeek502 force-pushed the std.process.Child branch 2 times, most recently from 76af53b to 1842b6d Compare May 23, 2024 21:13
@andrewrk
Copy link
Member Author

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
Copy link
Collaborator

squeek502 commented May 23, 2024

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 force-pushed the std.process.Child branch from 1842b6d to f47824f Compare May 26, 2024 16:32
@andrewrk
Copy link
Member Author

andrewrk commented May 26, 2024

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 :-)

@andrewrk andrewrk enabled auto-merge May 26, 2024 17:08
@andrewrk andrewrk merged commit 591bbaf into master May 26, 2024
10 checks passed
@andrewrk andrewrk deleted the std.process.Child branch May 26, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants