Skip to content

Use subprocess umask support#1754

Merged
juergbi merged 1 commit intoapache:masterfrom
nanonyme:pre-exec
Jan 11, 2025
Merged

Use subprocess umask support#1754
juergbi merged 1 commit intoapache:masterfrom
nanonyme:pre-exec

Conversation

@nanonyme
Copy link
Contributor

@nanonyme nanonyme commented Sep 16, 2022

This requires Python 3.9 or newer

@nanonyme nanonyme force-pushed the pre-exec branch 2 times, most recently from 9675062 to 91c47a3 Compare September 16, 2022 17:53
@nanonyme nanonyme marked this pull request as ready for review September 16, 2022 18:15
@nanonyme
Copy link
Contributor Author

nanonyme commented Sep 16, 2022

The leading idea here is that the time between fork and exec is perilous and we can avoid an unnecessary context switch from C code to Python code by using Python builtin umask setting functionality. The os.umask call itself is fine but the dance to make existing pre-exec function work through creating a Python wrapper means we do a context switch.

@nanonyme nanonyme marked this pull request as draft August 7, 2024 18:47
@nanonyme
Copy link
Contributor Author

nanonyme commented Aug 7, 2024

I will remove workaround and wait until BuildStream is ready to drop Python 3.8.

@nanonyme nanonyme changed the title Use subprocess umask support where available Use subprocess umask support Aug 7, 2024
@nanonyme nanonyme marked this pull request as ready for review December 11, 2024 20:49
Copy link
Contributor

@juergbi juergbi left a comment

Choose a reason for hiding this comment

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

Looks good to me. As the release process for 2.4.0 has already started, it may be a bit late to get this still in before the release, though.

@juergbi
Copy link
Contributor

juergbi commented Dec 20, 2024

We should be able to remove pylint: disable=subprocess-popen-preexec-fn from utils.py.

@nanonyme
Copy link
Contributor Author

@juergbi removed the stanza.

This requires Python 3.9 or newer
@juergbi juergbi merged commit 504e442 into apache:master Jan 11, 2025
17 checks passed
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.

2 participants