Skip to content

Conversation

@caiiiycuk
Copy link
Contributor

@caiiiycuk caiiiycuk commented Oct 25, 2023

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@2c0fb8c). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #6051   +/-   ##
=======================================
  Coverage        ?   42.46%           
=======================================
  Files           ?      498           
  Lines           ?    75709           
  Branches        ?    11841           
=======================================
  Hits            ?    32147           
  Misses          ?    40409           
  Partials        ?     3153           

@caiiiycuk caiiiycuk force-pushed the main branch 2 times, most recently from 30eaee8 to 40e0744 Compare October 25, 2023 07:33
@caiiiycuk caiiiycuk changed the title Fix #6047,5271: Support one-line-one-function file format for asyncify lists Fix #6047, #5271: Support one-line-one-function file format for asyncify lists Oct 25, 2023
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm with some minor suggestions.

… file format for asyncify lists

Co-authored-by: Alon Zakai <[email protected]>
@caiiiycuk
Copy link
Contributor Author

@kripken I squashed commits into one. Do you think it is worst to change

needToHandleBracketingOperations = delim != "\n";

into opposite?

needToHandleBracketingOperations = delim == ",";

@kripken
Copy link
Member

kripken commented Oct 27, 2023

delim != "\n"; seems better to me, as newlines are special: they are the one case that we can just split naively using them, without scoping/bracketing.

@kripken kripken merged commit 7dcc532 into WebAssembly:main Oct 30, 2023
@kripken
Copy link
Member

kripken commented Nov 1, 2023

@caiiiycuk it looks like this breaks the auto-updater script,

$ ./scripts/auto_update_tests.py lit
[..]
Fatal: Failed opening '%S/asyncify-foo,bar-nl.txt'
[..]
subprocess.CalledProcessError: Command 'binaryen/scripts/foreach.py binaryen/test/lit/passes/asyncify_pass-arg=asyncify-onlylist@foo,bar.wast /tmp/tmptkbv0v5t wasm-opt --asyncify --pass-arg=asyncify-onlylist@@%S/asyncify-foo,bar-nl.txt -S -o -' returned non-zero exit status 1.
[..]
subprocess.CalledProcessError: Command '['/usr/bin/python3', 'binaryen/scripts/update_lit_checks.py', '--binaryen-bin=binaryen/bin', 'binaryen/test/lit/**/*.wast', 'binaryen/test/lit/**/*.wat']' returned non-zero exit status 1.

We should run that in CI, but don't atm, which is how this was missed.

What is %S trying to do there?

@caiiiycuk
Copy link
Contributor Author

caiiiycuk commented Nov 1, 2023

@kripken asyncify-foo,bar-nl.txt is located in the same folder as a test. However, if you just name it without specifying %S, it may not work correctly. This is because the current working directory is the root of the project or even some other folder. After briefly checking the documentation, it seems that %S refers to the source directory (the directory of the file currently being run), and this could potentially resolve the issue. I apologize for not testing it in different folders to confirm.

@caiiiycuk
Copy link
Contributor Author

Ah, it's not related to different folders. I didn't imagine that I could break the auto-update, but now it seems obvious. Sorry

@kripken
Copy link
Member

kripken commented Nov 1, 2023

@caiiiycuk I'm not sure I understand, but it sounds like you have an idea of what went wrong here - do you know how to fix it?

@caiiiycuk
Copy link
Contributor Author

@caiiiycuk caiiiycuk deleted the main branch November 18, 2023 16:26
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
…mbly#6051)

If there are newlines in the list, then we split using them in a simple manner
(that does not take into account nesting of any other delimiters).

Fixes WebAssembly#6047
Fixes WebAssembly#5271
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