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

Remove custom permission error code and restore path in read error #451

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

rwstauner
Copy link
Contributor

@rwstauner rwstauner commented Jul 21, 2023

The existing error message says

bootsnap doesn't have permission to write cache entries in '#{cache_path}'
(or, less likely, doesn't have permission to read '#{path}')"

but at this point i think the error will never be about the cache and only for the original file.

We stopped reporting write errors in
d878622
and read errors in
503e9d5

In that case it seems like we may as well remove the wrapper class and let the original error bubble up...
but when I did that the exception message was the code "bs_fetch:open_current_file:open" rather than mentioning what file path failed to be read.

My C is incredibly rusty; I don't know if this is a good idea.

What do you think? Do we want to do it this way?
Are there other places we should replace the "bs:*" code with the file path (like the fstat that follows?)

Another option would be to keep the ruby method and just simplify the message
(in which case the C code wouldn't have to change).

@casperisfine casperisfine self-requested a review July 24, 2023 07:59
@@ -368,7 +368,8 @@ open_current_file(char * path, struct bs_cache_key * key, const char ** errno_pr

fd = open(path, O_RDONLY);
if (fd < 0) {
*errno_provenance = "bs_fetch:open_current_file:open";
// ensure error for reading the input file contains the original path
*errno_provenance = path;
Copy link
Contributor

Choose a reason for hiding this comment

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

So open_current_file is only called from bs_fetch, and path here is RSTRING_PTR of the path_v bs_fetch received.

If we return fd <0, bs_fetch jumps to the fail_errno label, so we're not actually giving more information to the caller here, since it already have path_v on its stack.

If we wish to augment or change the error message, we can directly do it there.

Copy link
Contributor Author

@rwstauner rwstauner Jul 24, 2023

Choose a reason for hiding this comment

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

Thanks, I like that idea.

In bs_fetch we will goto fail_errno in several cases:

I updated the code to set the message to the file the operation was on, but I may have gone a little overboard. 😅

Add a test to prove that if there is an error reading a file
the message contains the problematic path.

The custom permission error class and method were designed
to warn about errors with the cache path
but we stopped reporting write errors in
d878622
and read errors in
503e9d5
@casperisfine casperisfine merged commit 2ea29be into main Jul 25, 2023
17 checks passed
@casperisfine
Copy link
Contributor

I like it, thanks.

@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 25, 2023 06:42 Inactive
@rwstauner rwstauner deleted the rwstauner/permission-error branch July 25, 2023 14:17
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.

None yet

2 participants