Skip to content

Give better errors for some permission issues#4965

Merged
deivid-rodriguez merged 4 commits intomasterfrom
permission_stuff
Oct 14, 2021
Merged

Give better errors for some permission issues#4965
deivid-rodriguez merged 4 commits intomasterfrom
permission_stuff

Conversation

@deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Oct 7, 2021

What was the end-user or developer problem that led to this PR?

Sometimes bundler fails to do what it needs to do regarding some file operations like creating the gem folder where a gem will be installed, or deleting the previous installation. In some of those cases, bundler fails to report a proper error and instead prints a bug report template with a different error at random places.

What is your fix for the problem, implemented in this PR?

The fix is to properly detect where the actual problem happens, and give a proper error.

Closes #4888.
Closes #4938.
Closes #4939.
Closes #4981.

Make sure the following tasks are checked

@deivid-rodriguez
Copy link
Contributor Author

I'd like to ask for some opinions here. Is it to much to automatically grant permissions on the installation folder if we can?

@deivid-rodriguez
Copy link
Contributor Author

This a weird MacOS failure (only happens on 2.5, 2.6, and 2.7, but not on 3.0) that I need to investigate. But in addition to that, I'm going to tweak the fix.

I think it's fine that bundler tries to automatically fix permissions so that it's able to delete an existing gem folder before reinstalling a gem. But this shouldn't be the default FileUtils.rm_rf behaviour, since it should just be a wrapper on top of rm -rf and not deviate from the standard shell behavior.

So for now, I won't be changing fileutils at all. Instead, I will check whether the folder has actually been removed after FileUtils.rm_rf and if not, retry with fixed permissions.

@deivid-rodriguez
Copy link
Contributor Author

I updated this PR with the more minimal approach to improve the issue. We don't automatically try to fix any permissions for now, but we now give a proper error instead of throwing a bug report template at random places.

I'm setting this PR to close all permission issues. We'll probably get new issues but with better errors where the culprit will be easier to investigate and what to do about them will be more clear.

@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review October 14, 2021 10:48
@deivid-rodriguez deivid-rodriguez changed the title Fix weird permission errors Give better errors for some permission issues Oct 14, 2021
Instead of showing the bug report template with an error at a random
place.
@deivid-rodriguez deivid-rodriguez merged commit 4b5ffab into master Oct 14, 2021
@deivid-rodriguez deivid-rodriguez deleted the permission_stuff branch October 14, 2021 17:56
deivid-rodriguez added a commit that referenced this pull request Oct 25, 2021
Give better errors for some permission issues

(cherry picked from commit 4b5ffab)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants