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

Convert adapters to Python #48

Merged
merged 11 commits into from
Feb 13, 2023
Merged

Convert adapters to Python #48

merged 11 commits into from
Feb 13, 2023

Conversation

evacchi
Copy link
Contributor

@evacchi evacchi commented Jan 26, 2023

While setting up CI on GHA for wazero to run against the test suite, we realized that the Python script is assuming a Unix environment. I am opening this PR to start a discussion on the best approach. In my own tests, this worked reliably.

This PR:

  1. allows colorama>=0.4.3, otherwise pip on Windows (GHA windows-2022) raises the error:
INFO: pip is looking at multiple versions of colorama to determine which version is compatible with other requirements. This could take a while.
ERROR: Cannot install -r requirements/dev.txt (line 4) and colorama==0.4.3 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested colorama==0.4.3
    pylint 2.14.3 depends on colorama>=0.4.5; sys_platform == "win32"
  1. converts the shell-based adapters to Python and invokes them through the current Python interpreter (in a new sub-process)

  2. updates docs and GitHub actions accordingly

(Originally proposed changes – no longer apply) 2. we explicitly invoke the shell adapter through `bash` instead of relying on the `#!` directive, because on Windows that won't work. Of course this also means that all adapters would be now expected to be runnable on bash.
  1. as a further degree of complexity, it looks like proper bash (installed under C:\Program Files\Git\bin is not on the PATH when invoked from Python on GHA, invoking instead C:\Windows\system32\bash.exe which is just a stub that prints a message telling the user to install WSL (which of course we don't want as that's a virtualized linux distro).

Instead of fiddling with PATH, I decided to export an env variable (TEST_SHELL_RUNNER), which defaults to bash that can be overridden to a full path to a shell executable.

EDIT: minimal reproducer https://github.com/evacchi/gha-playground/actions/runs/4013516139/jobs/6892946727

EDIT2: I found a better workaround actions/runner-images#1081 (comment)

Signed-off-by: Edoardo Vacchi evacchi@users.noreply.github.com

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi evacchi changed the title Ensure scripts are runnable on Windows: expose env var Ensure scripts are runnable on Windows Jan 26, 2023
@loganek
Copy link
Collaborator

loganek commented Jan 28, 2023

Thanks a lot for the change. I do understand the motivation and I think it makes sense for Github Workflow. However, this change adds a requirement that all the adapters now have to be bash scripts. I'd rather avoid that unless it's absolutely necessary. Could we potentially solve it in a way so it doesn't force adapter maintainers to only write bash scripts?

@evacchi
Copy link
Contributor Author

evacchi commented Jan 29, 2023

Obviously fair point. I have thought of two possible solutions. Neither is great 😅

  1. parse the shebang. Kind of a hack. Moreover either we expect the shebang to be of the form

     !#bash
    

i.e. a bare executable without leading path, or we'll also need to parse the path (of course /bin/sh won't make any sense on windows)

  1. add a tiny config file next to the adapter. While this feels less hackish it is basically reproducing the shebang, including the path issue

While I would favor (2) it's only slightly less of a hack

@codefromthecrypt
Copy link

PS after this change, there's still a problem with the shell scripts (at least in git bash). For example, assemblyscript environ_get-multiple-variables fails when run in anything including wasmtime due to the parse arg loop not handling newlines properly.

It may be a better choice to not use bash and also python, rather just python? In other words maybe we can avoid the whole fork bash problems by not using it and instead have the shims python?

$ python3 test-runner/wasi_test_runner.py -t ./tests/assemblyscript/testsuite/ -r adapters/wasmtime.sh
--snip--
Test environ_get-multiple-variables failed
  [exit_code] 0 == 1
STDOUT
Unknown option line
--snip--

@evacchi
Copy link
Contributor Author

evacchi commented Jan 30, 2023

using Python would seem fair to me. After all the adapters are pretty simple

@loganek
Copy link
Collaborator

loganek commented Feb 3, 2023

I'd be ok with using Python as adapter, given it's very simple and we already require it for runtime anyway. @evacchi would you like to update your PR to support Python for adapters?

@evacchi
Copy link
Contributor Author

evacchi commented Feb 3, 2023

I'd be ok with using Python as adapter, given it's very simple and we already require it for runtime anyway. @evacchi would you like to update your PR to support Python for adapters?

so translate the other adapters? sure that's doable if you're ok with it :)

@evacchi
Copy link
Contributor Author

evacchi commented Feb 6, 2023

I have added another commit (we can close/squash/whatever the PR in case this looks good) to run python-based adapters.

Copy link
Collaborator

@loganek loganek left a comment

Choose a reason for hiding this comment

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

Thanks a lot, the change looks good to me. I left a few minor comments. i think we should also delete bash adapters once we have python ones too.

I think we should also update the documentation and and CI jobs. Please let me know if you can help with those as well.

@evacchi
Copy link
Contributor Author

evacchi commented Feb 8, 2023

Wonderful, sure will look into that too. In the meantime I am also making super sure everything works as expected

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi evacchi requested a review from loganek February 12, 2023 15:40
@evacchi evacchi changed the title Ensure scripts are runnable on Windows Convert adapters to Python Feb 12, 2023
@evacchi
Copy link
Contributor Author

evacchi commented Feb 12, 2023

I think I have applied the changes you suggested; obviously feel free to add more comments :) in case all is good, feel also free to squash the PR and merge (unless we want me to rebase and squash explicitly)

Thanks!

Copy link
Collaborator

@loganek loganek left a comment

Choose a reason for hiding this comment

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

The change looks good. I don't know of any other runtime that implements the adapter (other than toywasm, which already is written in Python), so I think we can merge it now.

Many thanks for making the change!

@loganek loganek merged commit dc7f8d2 into WebAssembly:main Feb 13, 2023
@evacchi evacchi deleted the win-support branch February 13, 2023 11:19
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