Skip to content
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

Do not chdir before calling posix_spawn #4606

Merged
merged 11 commits into from
Jun 14, 2023

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Jun 22, 2022

We allow users to set the working directory of the child process when
they use the Process API. Unfortunately, this was implemented by calling
chdir in the parent process, which has nasty global effects on the
process and cannot safely be used concurrently.

glibc has a non-POSIX extension to posix_spawn that can call chdir in
the child process, so we add that call instead where it's available, and
add a regression test.

Resolves swiftlang/swift#59610

@Lukasa
Copy link
Contributor Author

Lukasa commented Jun 22, 2022

@swift-ci please test

@Lukasa
Copy link
Contributor Author

Lukasa commented Jun 22, 2022

Woof, compile failures. I'll try to chase this up.

@Lukasa
Copy link
Contributor Author

Lukasa commented Jun 22, 2022

@swift-ci please test

@Lukasa
Copy link
Contributor Author

Lukasa commented Jun 22, 2022

Blocked by swiftlang/swift#59643.

@millenomi
Copy link
Contributor

Please test with swiftlang/swift#59643
@swift-ci please test

@millenomi
Copy link
Contributor

LGTM

@Lukasa
Copy link
Contributor Author

Lukasa commented Jun 23, 2022

@swift-ci please test

1 similar comment
@Lukasa
Copy link
Contributor Author

Lukasa commented Jun 24, 2022

@swift-ci please test

@tomerd
Copy link

tomerd commented Jul 21, 2022

@Lukasa this would be nice to get into 5.6.3: https://forums.swift.org/t/development-open-for-swift-5-6-3-for-linux-and-windows/58859

Can we get CI green so its mergable?

cc @parkera @shahmishal

@Lukasa
Copy link
Contributor Author

Lukasa commented Jul 22, 2022

I'll do my best but I'm running into awkwardness around the headers that I'll need to fix.

@tomerd
Copy link

tomerd commented Aug 26, 2022

@Lukasa this would be nice to get into 5.7

Can we get CI green so its mergable and brought over the the 5.7 branch?

We allow users to set the working directory of the child process when
they use the Process API. Unfortunately, this was implemented by calling
chdir in the parent process, which has nasty global effects on the
process and cannot safely be used concurrently.

glibc has a non-POSIX extension to posix_spawn that can call chdir in
the child process, so we add that call instead where it's available, and
add a regression test.
@Lukasa Lukasa force-pushed the cb-no-chdir-on-process branch from 90c21f5 to 4811dbb Compare November 18, 2022 12:43
@Lukasa
Copy link
Contributor Author

Lukasa commented Nov 18, 2022

@swift-ci test

@Lukasa
Copy link
Contributor Author

Lukasa commented Nov 18, 2022

I've brought this up to date with main and should hopefully have the tests passing now.

@Lukasa
Copy link
Contributor Author

Lukasa commented Nov 18, 2022

@tomerd how would you like to manage backporting this?

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@tomerd
Copy link

tomerd commented Jun 13, 2023

i would love to see this in 5.9 and even back ported to 5.8

@tshortli
Copy link

There's a build regression related to this change on Ubuntu: #4762

@shahmishal
Copy link
Member

@Lukasa We are going revert this change to unblock nightly toolchains. #4764

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

Successfully merging this pull request may close these issues.

Process.currentDirectoryURL affects global state on Linux
8 participants