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

syscallx: use golang.org/x/sys #149

Closed
wants to merge 1 commit into from

Conversation

zhsj
Copy link
Contributor

@zhsj zhsj commented Jan 6, 2020

use Readlink from golang.org/x/sys/{unix,windows}

syscall_unix.go is also changed because golang.org/x/sys is
preferred than syscall package.

use Readlink from golang.org/x/sys/{unix,windows}

syscall_unix.go is also changed because golang.org/x/sys is
preferred than syscall package.

Signed-off-by: Shengjing Zhu <[email protected]>
@estesp
Copy link
Member

estesp commented Jan 6, 2020

Given this will no longer use the syscall import, I think it might be better to delete these files/this directory and move the call to sys/unix.Readlink into the sysx directory (where it is being used from file_posix.go); I guess you would need to create 2 files there given there is windows support, but maybe a readlink_unix.go and a readlink_windows.go in that directory with the calls to sys/unix and sys/windows respectively. I think this directory was temporary while the syscall itself was implemented for Windows before going upstream.

@zhsj
Copy link
Contributor Author

zhsj commented Jan 6, 2020

I read the code again, I find the duplicated Readlink on windows is intentional. So let's close this PR.
#113

@zhsj zhsj closed this Jan 6, 2020
@zhsj
Copy link
Contributor Author

zhsj commented Jan 6, 2020

Although @darstahl said the fork of Readlink was temporary, but I didn't find upstream commits addressing any bug. So maybe this is still not upstream yet.

@zhsj zhsj deleted the clean-up-syscallx branch January 7, 2020 05:57
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.

2 participants