-
-
Notifications
You must be signed in to change notification settings - Fork 330
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 for Opal::Cache::FileCache.dir_writable? #2529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, left a comment about windows compatibility and checking for the root path existence.
lib/opal/cache/file_cache.rb
Outdated
# Check if we can robustly mkdir_p a directory. | ||
def self.dir_writable?(*paths) | ||
dir = nil | ||
paths = paths.reduce([]) do |a, b| | ||
[*a, dir = a.last ? File.expand_path(b, a.last) : b] | ||
dir = "/" | ||
until paths.empty? | ||
dir = File.expand_path(paths.shift, dir) | ||
ok = File.directory?(dir) && File.writable?(dir) if File.exist?(dir) | ||
end | ||
|
||
File.exist?(paths.first) && | ||
paths.reverse.all? do |i| | ||
!File.exist?(i) || (File.directory?(i) && File.writable?(i)) | ||
end | ||
|
||
dir | ||
dir if ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
Since expand_path
accepts nil
as the second argument it can be skipped, which makes it compatible with windows, also added a check on the first path segment to ensure it exist, it being the "root".
def self.dir_writable?(*paths)
return false unless File.exist?(paths.first)
until paths.empty?
dir = File.expand_path(paths.shift, dir)
ok = File.directory?(dir) && File.writable?(dir) if File.exist?(dir)
end
dir if ok
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, Windows compatibility, didn't think of that one.
The PR is marked as "changes requested" but you already made the changes, so I'm not sure if you're waiting on something from me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't committed that change to the branch. We did now :) Merging in a moment
…gardless of the dir being writeable or not Co-authored-by: hmdne <54514036+hmdne@users.noreply.github.com> Co-authored-by: Elia <elia@schito.me>
which previously returned truthy regardless of the dir being writeable or not, resulting in
dir_writable?(Dir.home, '.cache', 'opal')
being always true, even for non-login user (with $HOME set to /nonexistent)