From 2083e6b04cbe9d453b87fc0174bc8743056635a6 Mon Sep 17 00:00:00 2001 From: Delyan Angelov Date: Thu, 20 Oct 2022 13:56:06 +0300 Subject: [PATCH] os: cleanup APIs returning `!bool` to either return `!` or `bool` (#16111) --- cmd/tools/vdoctor.v | 4 +- vlib/io/util/util.v | 19 ++++----- vlib/io/util/util_test.v | 6 +-- vlib/os/dir_expansions_test.v | 7 ++-- vlib/os/filelock/lib_nix.c.v | 12 +++--- vlib/os/filelock/lib_windows.c.v | 10 ++--- vlib/os/os.js.v | 7 ++++ vlib/os/os.v | 6 +++ vlib/os/os_js.js.v | 8 ++-- vlib/os/os_nix.c.v | 67 ++++++-------------------------- vlib/os/os_test.v | 2 +- vlib/os/os_windows.c.v | 28 ++++++------- vlib/v/builder/compile.v | 2 +- 13 files changed, 69 insertions(+), 109 deletions(-) diff --git a/cmd/tools/vdoctor.v b/cmd/tools/vdoctor.v index a0f963b783..f5243fa40b 100644 --- a/cmd/tools/vdoctor.v +++ b/cmd/tools/vdoctor.v @@ -257,8 +257,8 @@ fn (mut a App) report_info() { } fn is_writable_dir(path string) bool { - res := os.is_writable_folder(path) or { false } - return res + os.ensure_folder_is_writable(path) or { return false } + return true } fn main() { diff --git a/vlib/io/util/util.v b/vlib/io/util/util.v index 9aadca9b09..6b4f830861 100644 --- a/vlib/io/util/util.v +++ b/vlib/io/util/util.v @@ -19,7 +19,7 @@ pub fn temp_file(tfo TempFileOptions) !(os.File, string) { if d == '' { d = os.temp_dir() } - os.is_writable_folder(d) or { + os.ensure_folder_is_writable(d) or { return error(@FN + ' could not create temporary file in "$d". Please ensure write permissions.') } @@ -46,31 +46,28 @@ pub struct TempDirOptions { pattern string } +fn error_for_temporary_folder(fn_name string, d string) !string { + return error('$fn_name could not create temporary directory "$d". Please ensure you have write permissions for it.') +} + // temp_dir returns an uniquely named, writable, directory path pub fn temp_dir(tdo TempFileOptions) !string { mut d := tdo.path if d == '' { d = os.temp_dir() } - os.is_writable_folder(d) or { - return error(@FN + - ' could not create temporary directory "$d". Please ensure write permissions.') - } + os.ensure_folder_is_writable(d) or { return error_for_temporary_folder(@FN, d) } d = d.trim_right(os.path_separator) prefix, suffix := prefix_and_suffix(tdo.pattern) or { return error(@FN + ' $err.msg()') } for retry := 0; retry < util.retries; retry++ { path := os.join_path(d, prefix + random_number() + suffix) os.mkdir_all(path) or { continue } if os.is_dir(path) && os.exists(path) { - os.is_writable_folder(path) or { - return error(@FN + - ' could not create temporary directory "$d". Please ensure write permissions.') - } + os.ensure_folder_is_writable(path) or { return error_for_temporary_folder(@FN, d) } return path } } - return error(@FN + - ' could not create temporary directory "$d". Retry limit ($util.retries) exhausted. Please ensure write permissions.') + return error('${@FN} could not create temporary directory "$d". Retry limit ($util.retries) exhausted.') } // * Utility functions diff --git a/vlib/io/util/util_test.v b/vlib/io/util/util_test.v index 18f01649d1..261afb3f50 100644 --- a/vlib/io/util/util_test.v +++ b/vlib/io/util/util_test.v @@ -81,11 +81,10 @@ fn test_temp_dir() { return } assert os.is_dir(path) - mut writable := os.is_writable_folder(path) or { + os.ensure_folder_is_writable(path) or { assert false return } - assert writable mut prev_path := path // Test pattern path = util.temp_dir( @@ -114,11 +113,10 @@ fn test_temp_dir() { } assert path != prev_path assert os.is_dir(path) - writable = os.is_writable_folder(path) or { + os.ensure_folder_is_writable(path) or { assert false return } - assert writable assert path.contains(tfolder) filename = os.file_name(path) for c in filename { diff --git a/vlib/os/dir_expansions_test.v b/vlib/os/dir_expansions_test.v index ba3e1e5992..eb6234b97a 100644 --- a/vlib/os/dir_expansions_test.v +++ b/vlib/os/dir_expansions_test.v @@ -13,13 +13,12 @@ fn test_tmpdir() { os.rm(tfile) or { panic(err) } } -fn test_is_writable_folder() { +fn test_ensure_folder_is_writable() { tmp := os.temp_dir() - f := os.is_writable_folder(tmp) or { + os.ensure_folder_is_writable(tmp) or { eprintln('err: $err') - false + assert false } - assert f } fn test_expand_tilde_to_home() { diff --git a/vlib/os/filelock/lib_nix.c.v b/vlib/os/filelock/lib_nix.c.v index 2d77edae21..99af905ccf 100644 --- a/vlib/os/filelock/lib_nix.c.v +++ b/vlib/os/filelock/lib_nix.c.v @@ -17,21 +17,19 @@ pub fn (mut l FileLock) unlink() { C.unlink(&char(l.name.str)) } -pub fn (mut l FileLock) acquire() !bool { +pub fn (mut l FileLock) acquire() ! { if l.fd != -1 { - // lock already acquired by this instance - return false + return error_with_code('lock already acquired by this instance', 1) } fd := open_lockfile(l.name) if fd == -1 { - return error('cannot create lock file $l.name') + return error_with_code('cannot create lock file $l.name', -1) } if C.flock(fd, C.LOCK_EX) == -1 { C.close(fd) - return error('cannot lock') + return error_with_code('cannot lock', -2) } l.fd = fd - return true } pub fn (mut l FileLock) release() bool { @@ -44,7 +42,7 @@ pub fn (mut l FileLock) release() bool { return false } -pub fn (mut l FileLock) wait_acquire(s int) !bool { +pub fn (mut l FileLock) wait_acquire(s int) bool { fin := time.now().add(s) for time.now() < fin { if l.try_acquire() { diff --git a/vlib/os/filelock/lib_windows.c.v b/vlib/os/filelock/lib_windows.c.v index 1e45082911..c81867c532 100644 --- a/vlib/os/filelock/lib_windows.c.v +++ b/vlib/os/filelock/lib_windows.c.v @@ -15,17 +15,15 @@ pub fn (mut l FileLock) unlink() { C.DeleteFileW(t_wide) } -pub fn (mut l FileLock) acquire() !bool { +pub fn (mut l FileLock) acquire() ! { if l.fd != -1 { - // lock already acquired by this instance - return false + return error_with_code('lock already acquired by this instance', 1) } fd := open(l.name) if fd == -1 { - return error('cannot create lock file $l.name') + return error_with_code('cannot create lock file $l.name', -1) } l.fd = fd - return true } pub fn (mut l FileLock) release() bool { @@ -39,7 +37,7 @@ pub fn (mut l FileLock) release() bool { return false } -pub fn (mut l FileLock) wait_acquire(s int) !bool { +pub fn (mut l FileLock) wait_acquire(s int) bool { fin := time.now().add(s) for time.now() < fin { if l.try_acquire() { diff --git a/vlib/os/os.js.v b/vlib/os/os.js.v index e47ffbd737..e16f27a21f 100644 --- a/vlib/os/os.js.v +++ b/vlib/os/os.js.v @@ -177,3 +177,10 @@ pub fn file_last_mod_unix(path string) int { return mtime } + +pub fn ensure_folder_is_writable(path string) ! { + fpath := join_path(path, 'some_newfile') + #try { $fs.writeFileSync(fpath); $fs.unlinkSync(fpath) } catch(e) { return error(new string('could not write to ' + path)) } + + _ := fpath +} diff --git a/vlib/os/os.v b/vlib/os/os.v index 0c6e40e918..c8ef218c8b 100644 --- a/vlib/os/os.v +++ b/vlib/os/os.v @@ -856,3 +856,9 @@ pub fn config_dir() !string { } return error('Cannot find config directory') } + +[deprecated: 'use os.ensure_folder_is_writable instead'] +pub fn is_writable_folder(folder string) !bool { + ensure_folder_is_writable(folder)! + return true +} diff --git a/vlib/os/os_js.js.v b/vlib/os/os_js.js.v index e0a41e8841..43ec29034e 100644 --- a/vlib/os/os_js.js.v +++ b/vlib/os/os_js.js.v @@ -1,15 +1,15 @@ module os -pub fn mkdir(path string, params MkdirParams) !bool { +pub fn mkdir(path string, params MkdirParams) ! { $if js_node { if path == '.' { - return true + return } #$fs.mkdirSync(path.valueOf()) - return true + return } $else { - return false + return error('could not create folder') } } diff --git a/vlib/os/os_nix.c.v b/vlib/os/os_nix.c.v index b04f12e3af..9d7333f9e4 100644 --- a/vlib/os/os_nix.c.v +++ b/vlib/os/os_nix.c.v @@ -281,51 +281,16 @@ pub fn ls(path string) ![]string { return res } -/* -pub fn is_dir(path string) bool { - //$if linux { - //C.syscall(4, path.str) // sys_newstat - //} - dir := C.opendir(path.str) - res := !isnil(dir) - if res { - C.closedir(dir) - } - return res -} -*/ - // mkdir creates a new directory with the specified path. -pub fn mkdir(path string, params MkdirParams) !bool { +pub fn mkdir(path string, params MkdirParams) ! { if path == '.' { - return true + return } - /* - mut k := 0 - defer { - k = 1 - } - */ apath := real_path(path) - // defer { - // apath.free() - //} - /* - $if linux { - $if !android { - ret := C.syscall(sys_mkdir, apath.str, params.mode) - if ret == -1 { - return error(posix_get_error_msg(C.errno)) - } - return true - } - } - */ r := unsafe { C.mkdir(&char(apath.str), params.mode) } if r == -1 { return error(posix_get_error_msg(C.errno)) } - return true } // execute starts the specified command, waits for it to complete, and returns its output. @@ -423,18 +388,18 @@ pub fn (mut c Command) close() ! { } } -pub fn symlink(origin string, target string) !bool { +pub fn symlink(origin string, target string) ! { res := C.symlink(&char(origin.str), &char(target.str)) if res == 0 { - return true + return } return error(posix_get_error_msg(C.errno)) } -pub fn link(origin string, target string) !bool { +pub fn link(origin string, target string) ! { res := C.link(&char(origin.str), &char(target.str)) if res == 0 { - return true + return } return error(posix_get_error_msg(C.errno)) } @@ -449,14 +414,6 @@ pub fn (mut f File) close() { return } f.is_opened = false - /* - $if linux { - $if !android { - C.syscall(sys_close, f.fd) - return - } - } - */ C.fflush(f.cfile) C.fclose(f.cfile) } @@ -475,14 +432,15 @@ pub fn debugger_present() bool { fn C.mkstemp(stemplate &u8) int -// `is_writable_folder` - `folder` exists and is writable to the process +// ensure_folder_is_writable checks that `folder` exists, and is writable to the process +// by creating an empty file in it, then deleting it. [manualfree] -pub fn is_writable_folder(folder string) !bool { +pub fn ensure_folder_is_writable(folder string) ! { if !exists(folder) { - return error('`$folder` does not exist') + return error_with_code('`$folder` does not exist', 1) } if !is_dir(folder) { - return error('`$folder` is not a folder') + return error_with_code('`$folder` is not a folder', 2) } tmp_perm_check := join_path_single(folder, 'XXXXXX') defer { @@ -491,12 +449,11 @@ pub fn is_writable_folder(folder string) !bool { unsafe { x := C.mkstemp(&char(tmp_perm_check.str)) if -1 == x { - return error('folder `$folder` is not writable') + return error_with_code('folder `$folder` is not writable', 3) } C.close(x) } rm(tmp_perm_check)! - return true } [inline] diff --git a/vlib/os/os_test.v b/vlib/os/os_test.v index 70baf18282..668ef27af1 100644 --- a/vlib/os/os_test.v +++ b/vlib/os/os_test.v @@ -469,7 +469,7 @@ fn test_realpath_absolutepath_symlink() { file_name := 'tolink_file.txt' symlink_name := 'symlink.txt' create_file(file_name)! - assert os.symlink(file_name, symlink_name)! + os.symlink(file_name, symlink_name)! rpath := os.real_path(symlink_name) println(rpath) assert os.is_abs_path(rpath) diff --git a/vlib/os/os_windows.c.v b/vlib/os/os_windows.c.v index e35a67c44d..c1adb9fe5b 100644 --- a/vlib/os/os_windows.c.v +++ b/vlib/os/os_windows.c.v @@ -190,16 +190,15 @@ pub fn ls(path string) ![]string { } // mkdir creates a new directory with the specified path. -pub fn mkdir(path string, params MkdirParams) !bool { +pub fn mkdir(path string, params MkdirParams) ! { if path == '.' { - return true + return } apath := real_path(path) if !C.CreateDirectory(apath.to_wide(), 0) { return error('mkdir failed for "$apath", because CreateDirectory returned: ' + get_error_msg(int(C.GetLastError()))) } - return true } // Ref - https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=vs-2019 @@ -378,7 +377,7 @@ pub fn raw_execute(cmd string) Result { } } -pub fn symlink(origin string, target string) !bool { +pub fn symlink(origin string, target string) ! { // this is a temporary fix for TCC32 due to runtime error // TODO: find the cause why TCC32 for Windows does not work without the compiletime option $if x64 || x32 { @@ -397,12 +396,12 @@ pub fn symlink(origin string, target string) !bool { if !exists(target) { return error('C.CreateSymbolicLinkW reported success, but symlink still does not exist') } - return true + return } - return false + return error('could not symlink') } -pub fn link(origin string, target string) !bool { +pub fn link(origin string, target string) ! { res := C.CreateHardLinkW(target.to_wide(), origin.to_wide(), C.NULL) // 1 = success, != 1 failure => https://stackoverflow.com/questions/33010440/createsymboliclink-on-windows-10 if res != 1 { @@ -411,7 +410,6 @@ pub fn link(origin string, target string) !bool { if !exists(target) { return error('C.CreateHardLinkW reported success, but link still does not exist') } - return true } pub fn (mut f File) close() { @@ -494,19 +492,21 @@ pub fn loginname() string { return unsafe { string_from_wide(&loginname[0]) } } -// `is_writable_folder` - `folder` exists and is writable to the process -pub fn is_writable_folder(folder string) !bool { +// ensure_folder_is_writable checks that `folder` exists, and is writable to the process +// by creating an empty file in it, then deleting it. +pub fn ensure_folder_is_writable(folder string) ! { if !exists(folder) { - return error('`$folder` does not exist') + return error_with_code('`$folder` does not exist', 1) } if !is_dir(folder) { - return error('`folder` is not a folder') + return error_with_code('`folder` is not a folder', 2) } tmp_folder_name := 'tmp_perm_check_pid_' + getpid().str() tmp_perm_check := join_path_single(folder, tmp_folder_name) - write_file(tmp_perm_check, 'test') or { return error('cannot write to folder "$folder": $err') } + write_file(tmp_perm_check, 'test') or { + return error_with_code('cannot write to folder "$folder": $err', 3) + } rm(tmp_perm_check)! - return true } [inline] diff --git a/vlib/v/builder/compile.v b/vlib/v/builder/compile.v index 91c3874332..c65798bd79 100644 --- a/vlib/v/builder/compile.v +++ b/vlib/v/builder/compile.v @@ -31,7 +31,7 @@ fn check_if_output_folder_is_writable(pref &pref.Preferences) { if odir.len == pref.out_name.len { output_folder = os.getwd() } - os.is_writable_folder(output_folder) or { + os.ensure_folder_is_writable(output_folder) or { // An early error here, is better than an unclear C error later: verror(err.msg()) }