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.

Sign up for GitHub

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

Jump to bottom

feat: support for Windows #228

Merged
merged 5 commits into from
May 23, 2022
Merged

feat: support for Windows #228

merged 5 commits into from
May 23, 2022

Conversation

ii64
Copy link
Contributor

@ii64 ii64 commented May 17, 2022

  • fix (loader): default loader now has build tag: for linux and darwin
  • feat (loader): add memory allocator support for Windows

AsterDY
AsterDY reviewed May 19, 2022
View reviewed changes
Copy link
Collaborator

@AsterDY AsterDY left a comment •
edited
Loading

Choose a reason for hiding this comment

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

please use original format:

  1. indent: 'four space' instead of 'tab'
  2. import quote: `` instead of ""

@ii64
feat: support for Windows
7566192 
* fix (loader): default loader now has build tag: for linux and darwin
* feat (loader): add memory allocator support for Windows
@ii64 ii64 force-pushed the feat/support_os_windows branch from 59b1b38 to 7566192 Compare May 19, 2022 11:34
@ii64
Copy link
Contributor Author

ii64 commented May 19, 2022

Fixed

AsterDY
AsterDY reviewed May 19, 2022
View reviewed changes
Copy link
Collaborator

@AsterDY AsterDY left a comment

Choose a reason for hiding this comment

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

Could you please add a CI file (ex: https://github.com/bytedance/sonic/blob/main/.github/workflows/push-check-go117.yml) to test if it works? use 'windows-latest' as basic image maybe?

internal/loader/loader_windows_amd64.go Outdated Show resolved Hide resolved
ii64 added 2 commits May 19, 2022 21:05
@ii64
feat: add Windows CI
b78e9d2 
Added Windows CI for GitHub Actions
Go version: 1.15.x, 1.16.x, 1.17.x, 1.18.x
@ii64
fix (loader): remove amd64 build constraint from Windows
6b6e4eb
@ii64
Copy link
Contributor Author

ii64 commented May 19, 2022

All test case passed on my machine, except for issue_test/plugin_test.go.
The Windows CI got ThreadSanitizer error (error code: 1455), maybe OOM?

@AsterDY
Copy link
Collaborator

AsterDY commented May 20, 2022 •
edited
Loading

It seems like go racer's problem: golang/go#46099, and doesn't get fixed yet. Maybe removing -race test flag is the only feasible solution at present

@ii64
fix: TSAN error on Windows CI
33d7387 
Temporary remove `-race` flag

Link: golang/go#46099
@ii64
Copy link
Contributor Author

ii64 commented May 20, 2022 •
edited
Loading

Ouch, the GitHub Windows CI host got OOM, it has 7 GB RAM. I can reproduce TSAN error on my machine with -race flag, removing it fixed the problem. On issue_test, using -race took ~20x more RAM usage, could it might pass the test if memory/page were still available?

Attached the ut logs:
ut_norace_ok.log
ut_race_err_tsan.log
ut_race_err_tsan_2.log

@AsterDY AsterDY requested a review from chenzhuoyu May 21, 2022 07:38
@AsterDY AsterDY merged commit 75b728b into bytedance:main May 23, 2022
@AsterDY
Copy link
Collaborator

AsterDY commented May 23, 2022

Thanks to @ii64! This is the first effective PR from an outside developer! 🎉

@ii64 ii64 deleted the feat/support_os_windows branch August 22, 2022 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AsterDY AsterDY AsterDY approved these changes

@chenzhuoyu chenzhuoyu Awaiting requested review from chenzhuoyu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ii64 @AsterDY