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

copy_file implemented for Windows, Linux and Darwin #3186

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Smilex
Copy link

@Smilex Smilex commented Feb 11, 2024

I am unable to test it on darwin, but I assumed it's the same as Linux, so I copied it

Linux tested with WSL
Windows tested on Windows 11

This function was inspired by - https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-copyfile

Copy link
Contributor

@Tetralux Tetralux left a comment

Choose a reason for hiding this comment

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

Few things to fix up, otherwise decent.

core/os/os_darwin.odin Show resolved Hide resolved
core/os/file_windows.odin Outdated Show resolved Hide resolved
core/os/os_linux.odin Show resolved Hide resolved
core/os/file_windows.odin Outdated Show resolved Hide resolved
@Kelimion
Copy link
Member

It would be nice if on *nix platforms it'd copy permissions (chmod) and ownership (chown) instead of assigning the new file to the current user with some default permissions, or being able to pass in the desired owner and permission.

@Smilex
Copy link
Author

Smilex commented Feb 11, 2024

It would be nice if on *nix platforms it'd copy permissions (chmod) and ownership (chown) instead of assigning the new file to the current user with some default permissions, or being able to pass in the desired owner and permission.

I did stat for and copy the permissions at first, but decided to simplify
I'm not sure which I prefer

@Smilex
Copy link
Author

Smilex commented Feb 13, 2024

It would be nice if on *nix platforms it'd copy permissions (chmod) and ownership (chown) instead of assigning the new file to the current user with some default permissions, or being able to pass in the desired owner and permission.

Should I make it possible to pass chmod and chown?

@flysand7
Copy link
Contributor

For a simple implementation copying the original permissions should be enough

@Smilex
Copy link
Author

Smilex commented Feb 14, 2024

For a simple implementation copying the original permissions should be enough

Ok, now I stat for the mode first and open the new file with that mode

@dvrd
Copy link

dvrd commented Feb 17, 2024

I tested the implementation on MacOS and it works as expected

@Smilex
Copy link
Author

Smilex commented Feb 17, 2024

I tested the implementation on MacOS and it works as expected

thanks!

@Yawning
Copy link
Contributor

Yawning commented Feb 21, 2024

Some thoughts:

  • The Linux and Darwin code could be simplified by using os/stream_from_handle and io/copy.
  • Should failure during the copy process, remove the partially copied file? Right now it does.

@laytan
Copy link
Sponsor Contributor

laytan commented Feb 21, 2024

You probably also want os.O_TRUNC for the destination file, so it truncates the file if it already exists, otherwise it would just add to it

@Tetralux
Copy link
Contributor

Tetralux commented Mar 18, 2024

Some thoughts:

  • The Linux and Darwin code could be simplified by using os/stream_from_handle and io/copy.
  • Should failure during the copy process, remove the partially copied file? Right now it does.

It currently doesn't remove a partially copied file - at least not explicitly.

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

7 participants