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

WIP: feat: support for Windows #191

Open
wants to merge 10 commits into
base: revert-178-feat/adjust_windows
Choose a base branch
from

Conversation

lllbbbyyy
Copy link

What type of PR is this?

feat


What this PR does / why we need it (en: English/zh: Chinese):


en: This pr is an adaptation modification made to netpoll in order to complete the GLCC competition title "providing windows support for kitex".

Changes made so far:

  1. No error will be reported on the Windows platform (merged with previous modifications), but the function is not fully realized.
  2. Added compatible cross-platform abstractions, such as using fdtype to avoid platform differences caused by int and handle types. The specific modifications include: abstraction of handle type, abstraction of iovec, and read and write functions (only recv and send are supported under the windows platform).
  3. Implemented the sys_exec tool under the windows platform, including GetSysFdPairs, writev, readv, sendmsg, etc.
  4. Encapsulate WSAPoll into epoll interface similar to unix system, and can be called normally by poll type in netpoll.
  5. Modified the unit tests related to sys_exec and poll so that they can support the testing of the windows platform, and the above modifications have passed the unit tests.

What needs to be done next:

  1. Further logic review and problem checking
  2. Cross-platform support for net_listener (the current problem is that net.TCPListener cannot be converted to a raw socket)

zh: 此pr是为了完成GLCC赛题“对kitex提供windows支持”而对于netpoll进行的适配性修改,目的是能够使得netpoll支持windows平台,目前是截至中期考核进行的一些成果。

到目前为止已经进行的改动:

  1. 在windows平台上不会报错(和此前的修改进行了合并),但是功能上并没有完全实现。
  2. 添加了具有兼容性的跨平台抽象,例如使用fdtype去避免int与handle类型带来的平台差异等。具体的修改内容包括:对句柄类型的抽象、对iovec的抽象、对于read以及write函数(windows平台下仅支持recv和send)。
  3. 实现了windows平台下的sys_exec工具,主要包括GetSysFdPairs、writev、readv、sendmsg等。
  4. 将WSAPoll封装成了类似unix系统的epoll接口,并且能够被netpoll中的poll类型正常调用。
  5. 修改了sys_exec、poll相关的单元测试,使其能够支持windows平台的测试,并且上述修改均通过了单元测试。

接下来需要完成的:

  1. 进一步的逻辑审查与问题检查
  2. net_listener的跨平台支持(目前遇到的问题是net.TCPListener无法转换为原始socket)

Which issue(s) this PR fixes:

Fixes cloudwego/kitex#469

Added cross-platform type abstractions, such as fdtype, etc.

It encapsulates system calls under the windows platform, such as sendmsg, etc.

Under the windows platform, an interface similar to epoll is implemented by encapsulating WSAPoll

Completed cross-platform porting for Poll

Make the project not report errors under the windows platform

The corresponding test cases are modified to support cross-platform testing without changing their functions

The implementation of Poll and the following passed the unit test

TODO: Cross-platform implementation and testing of listener
@CLAassistant
Copy link

CLAassistant commented Aug 19, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Hchenn
Copy link
Contributor

Hchenn commented Aug 19, 2022

hi,同学你好,感谢对 netpoll 的大功能贡献 ~

我大致看了下,PR 内容确实有点多,所以我觉得我们可以先在 PR 风格 和 代码风格 上达成一些共识,方便 PR 的 review:

  1. pr 方面,如果改动量很大,还请整理下 commit,最好能把 pr 拆成一些较小的逻辑集合,分别作为 commit,这样 reviewer 可以快速了解变更了哪些设计。

  2. 由于之前有个 mock windows(refactor: netpoll can be imported by other components under Windows and compiled without error #178),从 reviewer 视角看,我很难搞清当前哪些被 mock 哪些又被真正支持了,因此我创建了 revert(WIP: revert: "feat: adapt to Windows platform" #192),想请你把当前 pr rebase 到 (WIP: revert: "feat: adapt to Windows platform" #192) 后面,这样可以更好的了解当前 pr 的设计。

  3. 在代码文件上,文件名后缀建议使用 并列式 提高区分度(go 源码风格)(+build 作为辅助手段),file.go + file_windows.go 建议改为 file_linux.go + file_windows.go

  4. 如无必要,不要开辟新的文件前缀,cross_platform_xxx 应该隶属于 sys_xxx 范畴。

以上,感谢 ~

@Hchenn Hchenn changed the title feat: support for Windows WIP: feat: support for Windows Aug 19, 2022
@lllbbbyyy lllbbbyyy changed the base branch from develop to revert-178-feat/adjust_windows August 19, 2022 09:13
@lllbbbyyy
Copy link
Author

hi,同学你好,感谢对 netpoll 的大功能贡献 ~

我大致看了下,PR 内容确实有点多,所以我觉得我们可以先在 PR 风格 和 代码风格 上达成一些共识,方便 PR 的 review:

  1. pr 方面,如果改动量很大,还请整理下 commit,最好能把 pr 拆成一些较小的逻辑集合,分别作为 commit,这样 reviewer 可以快速了解变更了哪些设计。
  2. 由于之前有个 mock windows(feat: adapt to Windows platform #178),从 reviewer 视角看,我很难搞清当前哪些被 mock 哪些又被真正支持了,因此我创建了 revert(WIP: revert: "feat: adapt to Windows platform" #192),想请你把当前 pr rebase 到 (WIP: revert: "feat: adapt to Windows platform" #192) 后面,这样可以更好的了解当前 pr 的设计。
  3. 在代码文件上,文件名后缀建议使用 并列式 提高区分度(go 源码风格)(+build 作为辅助手段),file.go + file_windows.go 建议改为 file_linux.go + file_windows.go
  4. 如无必要,不要开辟新的文件前缀,cross_platform_xxx 应该隶属于 sys_xxx 范畴。

以上,感谢 ~

1 后续会注意的,后续的修改会尽量将拆分为功能明确且较短的commit,方便review
2 3 4 已经进行了相应的修改

@Hchenn
Copy link
Contributor

Hchenn commented Aug 19, 2022

1 后续会注意的,后续的修改会尽量将拆分为功能明确且较短的commit,方便review 2 3 4 已经进行了相应的修改

感谢 ~, 先开发吧,最后阶段再整理下 commit,比如 fix,debug 之类移除就好了

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[GLCC 赛题] 为 kitex 适配 Windows
3 participants