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

fix: set executable bit on brioche binary if not already set #112

Merged

Conversation

jaudiger
Copy link
Contributor

@jaudiger jaudiger commented Aug 5, 2024

The code changes in self_update.rs add logic to set the executable bit on the brioche binary if it's not already set. This ensures that the binary can be executed properly after a self update.

Resolve #95

The code changes in `self_update.rs` add logic to set the executable bit on the `brioche` binary if it's not already set. This ensures that the binary can be executed properly after a self update.

Signed-off-by: Jérémy Audiger <jeremy.audiger@icloud.com>
Copy link
Member

@kylewlacy kylewlacy left a comment

Choose a reason for hiding this comment

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

One tweak before this is ready to merge: setting the permissions should be done on the temp file before renaming it, not after renaming. This is so that, if the process is killed (say, with Ctrl-C), the result is either the old version of Brioche or the new version of Brioche with correct permissions

@jaudiger
Copy link
Contributor Author

jaudiger commented Aug 8, 2024

Sure, I’ll resolve when I come back !

Signed-off-by: Jérémy Audiger <jeremy.audiger@icloud.com>
Signed-off-by: Jérémy Audiger <jeremy.audiger@icloud.com>
@jaudiger
Copy link
Contributor Author

I moved the code to apply the file permissions if necessary to the temporary file.

Copy link
Member

@kylewlacy kylewlacy left a comment

Choose a reason for hiding this comment

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

Thanks for this! Finally had some time to sit down and test this out, LGTM 👍

@kylewlacy kylewlacy merged commit 3517aec into brioche-dev:main Aug 26, 2024
5 checks passed
@jaudiger jaudiger deleted the self-update-set-executable-permissions branch August 26, 2024 09:45
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.

Fix brioche self-update setting wrong permissions
2 participants