Skip to content

Add support for thread-safe chdir in process spawning #981

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 2, 2025
Merged

Add support for thread-safe chdir in process spawning #981

merged 1 commit into from
Jun 2, 2025

Conversation

jakepetroules
Copy link
Contributor

@jakepetroules jakepetroules commented Apr 30, 2025 •
edited
Loading

This is needed as a fallback to support Amazon Linux 2 and OpenBSD.

Closes #980

@jakepetroules
Copy link
Contributor Author

@swift-ci test

@jakepetroules
Copy link
Contributor Author

Draft for now as I'm not confident this is safe at all.

@jakepetroules jakepetroules requested review from owenv and dmbryson April 30, 2025 21:21
jakepetroules
jakepetroules commented Apr 30, 2025
View reviewed changes
@jakepetroules
Copy link
Contributor Author

@swift-ci test

@dmbryson
Copy link
Contributor

dmbryson commented May 8, 2025

Unfortunately I think this is likely to run afoul of open file descriptor inheritance. I think we may want to use a trampoline binary to accomplish this.

@jakepetroules
Copy link
Contributor Author

Unfortunately I think this is likely to run afoul of open file descriptor inheritance. I think we may want to use a trampoline binary to accomplish this.

I think that's true even for the current posix_spawn implementation because POSIX_SPAWN_CLOEXEC_DEFAULT is an Apple-specific extension.

@jakepetroules
Copy link
Contributor Author

@swift-ci test

@jakepetroules jakepetroules requested a review from weissi May 22, 2025 07:56
@jakepetroules jakepetroules force-pushed the threadsafe-chdir branch 2 times, most recently from 82decf8 to 6f0bc88 Compare May 22, 2025 08:03
@jakepetroules
Copy link
Contributor Author

@swift-ci test

@jakepetroules jakepetroules force-pushed the threadsafe-chdir branch 2 times, most recently from 7c02181 to 4d171f8 Compare June 2, 2025 01:13
@jakepetroules jakepetroules marked this pull request as ready for review June 2, 2025 01:17
@jakepetroules
Copy link
Contributor Author

@swift-ci test

@jakepetroules
Copy link
Contributor Author

OK, I'm fairly confident this is sufficiently close to the existing posix_spawn based code path in llbuild (acknowledging that the posix_spawn path is also probably not perfect in one way or another) so as to get us unblocked for Amazon Linux 2 and OpenBSD.

Please note that this does NOT change behavior for macOS, Linux with a sufficiently new (2.29+) Glibc, or any other platform which has posix_spawn_file_actions_addchdir[_np].

@jakepetroules
Copy link
Contributor Author

@swift-ci test

@jakepetroules
Copy link
Contributor Author

@swift-ci smoke test

@jakepetroules
Copy link
Contributor Author

@swift-ci test windows

@jakepetroules
Add support for thread-safe chdir in process spawning
b59cd6d 
This is needed as a fallback to support Amazon Linux 2 and OpenBSD.
@jakepetroules
Copy link
Contributor Author

@swift-ci smoke test

@jakepetroules
Copy link
Contributor Author

@swift-ci test windows

@jakepetroules
Copy link
Contributor Author

@swift-ci test windows platform

@jakepetroules jakepetroules merged commit fd935d3 into main Jun 2, 2025
4 checks passed
@jakepetroules jakepetroules deleted the threadsafe-chdir branch June 2, 2025 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmbryson dmbryson dmbryson approved these changes

@owenv owenv owenv approved these changes

@weissi weissi Awaiting requested review from weissi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Amazon Linux 2 support for llbuild
3 participants
@jakepetroules @dmbryson @owenv