Implemented deno.chmodSync for mac/linux os users. #673
Conversation
js/os_test.ts
Outdated
| const path = "/README.md"; | ||
| const mode = "777"; | ||
| deno.chmodSync(path, mode); | ||
| }); |
There was a problem hiding this comment.
It would be better to create a temp directory and use that for the chmod target.
js/os.ts
Outdated
| * | ||
| * chmodSync(path, mode); | ||
| */ | ||
| export function chmodSync(path: string, mode: string): void { |
There was a problem hiding this comment.
I think we should use an integer for the mode - as Node does - https://nodejs.org/api/fs.html#fs_file_modes
src/handlers.rs
Outdated
| .arg("-c") | ||
| .arg(final_cmd.to_string()) | ||
| .output() | ||
| .expect("failed to execute process") |
There was a problem hiding this comment.
Starting a new process to do chmod is much too much overhead. Please call the syscall directly.
|
I will start working on the suggested changes. |
|
@srijanreddy98 Looks like you pushed |
|
@kevinkassimo I removed them, but build fails at LINK ./deno |
src/handlers.rs
Outdated
| if cfg!(target_os = "windows") { | ||
| path_to_dynamic_lib = "/home/srijan/Documents/chmod.dll"; // This will change to the path where deno will be present | ||
| } else { | ||
| path_to_dynamic_lib = "/home/srijan/Documents/chmod.so"; // This will change to the path where deno will be present |
There was a problem hiding this comment.
Hi Srijan - Thanks for looking into this but we shouldn't need any DLL loading to make this work. I think everything is in Rust's std library.
There was a problem hiding this comment.
@srijanreddy98 probably could try std::fs::set_permissions
There was a problem hiding this comment.
@kevinkassimo I initially thought of using it but this method takes a permission struct which has only one method called set_readonly(x: bool). So you can only set permissions to -r--r--r-- or -rw-rw-rw-. So I was looking for other ways.
There was a problem hiding this comment.
@srijanreddy98 then probably PermissionExt is what you are looking for.
I believe OpenOptionsExt could also be used
There was a problem hiding this comment.
@kevinkassimo will it work for windows?
There was a problem hiding this comment.
@srijanreddy98 It's unix only.
|
@piscisaureus How should chmod operate on windows? |
src/handlers.rs
Outdated
| permissions.set_mode(mode); | ||
| let _result = fs::set_permissions(path, permissions)?; | ||
| Ok(None) | ||
| }())) |
There was a problem hiding this comment.
This code seems to be badly formatted. Please run ./tools/format.py
| const filename = deno.makeTempDirSync() + "/test.txt"; | ||
| deno.writeFileSync(filename, data, 0o666); | ||
| let fileInfo = deno.statSync(filename); | ||
| console.log(fileInfo.mode, 0o666); // This is printing 33206 438 |
There was a problem hiding this comment.
@ry deno.statSync(filename).mode is not working properly.
Checkout the line 802 in the build log Here.
802: 33206 438. This is coming from the line console.log(fileInfo.mode, 0o666); on which I am commenting.
I had initially opened an issue #756. At that time it worked on travis. Now it is not working on travis also. Can you check if you are also having the same behaviour.
There was a problem hiding this comment.
@srijanreddy98 You might want to apply a mask (0o777). For example in python
oct(33206) == '0100666'
33206 & 0o777 == 0o666
|
@srijanreddy98 Sorry for letting this fall to the bottom - I was waiting on a discussion about windows support. I think simply having this being a no-op on windows is the right behavior (or at least acceptable for now). @kevinkassimo has cleaned it up in #1088. I will land there. |
`#[cppgc]` return values were added in denoland/deno_core@a736d86 which made creation of cppgc backed objects possible without direct access to V8 APIs. This patch builds on top of that and adds `#[cppgc]` support for async ops.
I have implemented chmodSync for mac/linux os.
Prior art:
Go: func Chmod(name string, mode FileMode) error
Node: fs.chmod(file, mode, callback){
Python: os.chmod(path, mode);
pub fn set_permissions<P: AsRef>(path: P, perm: Permissions) -> Result<()>
Permissions struct in Rust doesn't allow anything apart from -r--r--r-- or -rw-rw-rw