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

Enable running spec tests on Windows #2423

Conversation

loganek
Copy link
Collaborator

@loganek loganek commented Aug 4, 2023

We don't enable those tests in CI yet as not all of them are passing

@loganek loganek force-pushed the loganek/enable-testsuite branch 7 times, most recently from 4648d85 to 5e9ec5c Compare August 4, 2023 11:24
@loganek loganek changed the title Enable spec running tests on Windows Enable running spec tests on Windows Aug 4, 2023
@loganek loganek force-pushed the loganek/enable-testsuite branch 2 times, most recently from 7835754 to 1a482d6 Compare August 4, 2023 15:50
@loganek
Copy link
Collaborator Author

loganek commented Aug 4, 2023

@wenyongh looks like the build failure is not related to this change. Could we possibly rebase the dev/wasi-libc-windows branch onto the main branch? I guess there were some changes in main that fix the issue.

@wenyongh
Copy link
Contributor

wenyongh commented Aug 6, 2023

@wenyongh looks like the build failure is not related to this change. Could we possibly rebase the dev/wasi-libc-windows branch onto the main branch? I guess there were some changes in main that fix the issue.

@loganek, done with PR #2426.

@loganek loganek force-pushed the loganek/enable-testsuite branch 4 times, most recently from 2840912 to 4119205 Compare August 7, 2023 13:45
@loganek
Copy link
Collaborator Author

loganek commented Aug 7, 2023

Thanks a lot @wenyongh , the PR is now ready for review.

We don't enable those tests in CI yet as not all of them are passing
@loganek loganek force-pushed the loganek/enable-testsuite branch 2 times, most recently from 7315912 to 68b6708 Compare August 7, 2023 16:30
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -101,35 +135,53 @@ def __init__(self, args, no_pty=False):
self.stdin = os.fdopen(master, 'r+b', 0)
self.stdout = self.stdin

if platform.system().lower() == "windows":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just be curious, is it possible to have a cross-platform solution for reading stdout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. The only cross-platform I could think of was by using a thread and a queue (just like it was implemented in this PR for Windows); but I did not use this technique for other operating systems because it turned to be significantly (2 - 3 x) slower, so I kept the approach with select for non-windows platforms.

@wenyongh wenyongh merged commit ea76300 into bytecodealliance:dev/wasi-libc-windows Aug 9, 2023
737 of 738 checks passed
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Update wamr-test-suites scripts to enable running spec tests on Windows.
We don't enable those tests in CI yet as not all of them are passing.
@loganek loganek deleted the loganek/enable-testsuite branch June 10, 2024 12:47
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.

3 participants