Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix
make THREAD_MODEL=posix
#311Fix
make THREAD_MODEL=posix
#311Changes from 19 commits
626362a
384da08
0e7956a
2ab390a
b2f40de
9d3b1e4
04f6c3f
d2d3e5c
83a80a7
247bfeb
0acb15f
240c35f
e74d263
0878de7
db89382
53a4091
6539705
3784170
2339e18
80d9a43
d5bf750
eb50290
1d6a27c
831de19
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming of these new files seems a little odd.. since
posix
doesn't just mean threads. I would hope we could usepthreads
here (or something like that), but that might mean changing the way we trigger the build. Something likemake WITH_PTHREADS=1
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do a follow-up to that later? It's quite convenient and clear to use
$(THREAD_MODEL)
throughout the Makefile...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not rename these expectations file just to rename them again later.
Can you find a way to at least leave the existing files in place?
How about something like this:
I know that isn't Makefile syntax but you get the idea. This way the old files stay in place, and the new files have more meaningful names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I tried this and the issue is that we are using
diff
recursively so a structure like the following doesn't really work:The posix/pthreads directory gets in the way of the
diff
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we'll need two separate directory trees to
diff
with. Here are some options:a. append
/pthreads
or/no-pthreads
to the expected directory path; this at least means that the naming is a bit more accurateb. append something, e.g.,
pthreads
, to the target, so we woulddiff
against eitherexpected/wasm32-wasi
orexpected/wasm32-wasi-pthreads
Large diffs are not rendered by default.