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

Test and lint examples #117

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Conversation

benbrandt
Copy link
Contributor

@benbrandt benbrandt commented Oct 26, 2024

Hi @dicej I attempted to start testing the examples and have run into some issues. With the current codebase, if I run the code in the readme, I get this error:

Error:

Caused by:
    0: error while executing at wasm backtrace:
           0: 0x1c1c10e - wit-component:shim!indirect-wasi:cli/environment@0.2.0-get-environment
           1: 0x173ca - wit-component:adapter:wasi_snapshot_preview1!wasi_snapshot_preview1::State::get_environment::hef57c6f7dba7ec20
           2: 0x175e1 - wit-component:adapter:wasi_snapshot_preview1!environ_sizes_get
           3: 0x1c1bc25 - wit-component:shim!adapt-wasi_snapshot_preview1-environ_sizes_get
           4: 0x1b5da81 - libc.so!__wasi_environ_sizes_get
           5: 0x1b5d976 - libc.so!__wasilibc_initialize_environ
           6: 0x1b5f6ef - libc.so!__wasilibc_initialize_environ_eagerly
           7: 0x1b576f7 - libc.so!__wasm_call_ctors
           8: 0x1b57a64 - libc.so!_initialize
           9: 0x1c1b1fd - __init!<wasm function 745>
    1: called trapping stub: wasi:cli/environment@0.2.0#get-environment

Since the adapter used for wasi is from Wasmtime 17, I thought that might be the issue, so I am now using the one from the crate wasi-preview1-component-adapter-provider to make sure we were using the latest one (and it makes some things a little simpler anyway)

However, now it seems that the component can't find the files being added for initialization...

Error: ModuleNotFoundError: No module named 'app'

Caused by:
    ModuleNotFoundError: No module named 'app'

Any ideas? I'm a little worried I broke componentize-py with dependency updates for anyone wanting to target wasi worlds... (we are using a custom world at work)
But I guess that is the point of adding these tests

addresses #15

@benbrandt
Copy link
Contributor Author

benbrandt commented Oct 28, 2024

I have traced it to this commit: f19009e

Some dependency here broke it, likely wasmtime, but I am not sure.

@dicej
Copy link
Collaborator

dicej commented Oct 28, 2024

@benbrandt thanks for investigating and narrowing down the regression. I'll take a look.

@benbrandt
Copy link
Contributor Author

I'm trying incrementing the wasi adapter version as well to minimize the change

@dicej
Copy link
Collaborator

dicej commented Oct 28, 2024

I think this might be due to confusion related to WASI 0.2.0 vs. 0.2.1 imports which will require updates to the add_wasi_and_stubs function. Working on a fix now.

@dicej
Copy link
Collaborator

dicej commented Oct 28, 2024

This should fix it: #120

@benbrandt
Copy link
Contributor Author

Awesome. I had a feeling it might have to do with wasi 0.2.x since wasmtime 25 introduced the next version. Thanks!

I'll rebase after you merge and see if I can push the tests further

@benbrandt benbrandt force-pushed the test-examples branch 2 times, most recently from cee84e5 to 70fd0c0 Compare October 28, 2024 18:43
@benbrandt
Copy link
Contributor Author

@dicej it seems that everything is working on main, but not if I update the adapter file as I am doing here... I'll try upgrading each version of the adapter file to see which one causes the break. but at least main is working again, thanks for the fix

@benbrandt benbrandt force-pushed the test-examples branch 4 times, most recently from bbdbe9f to 87d3c19 Compare October 29, 2024 09:18
@benbrandt
Copy link
Contributor Author

@dicej Ok I added some basic smoke tests of what is in the readmes.

I am not always confident in my bash skills, so if you see anything obvious where this wouldn't catch errors, let me know.

For example, I wasn't sure how best to handle the http proxy or tcp examples, so I am just making sure they build for now.

@benbrandt benbrandt marked this pull request as ready for review October 29, 2024 09:21
@benbrandt benbrandt changed the title Test examples Test and lint examples Oct 29, 2024
@benbrandt
Copy link
Contributor Author

benbrandt commented Oct 29, 2024

Also attempted to address #37

I made some changes to see if we could pass "strict" mode if possible so we hopefully don't get flagged in someone else's mypy config

{docs}{NOT_IMPLEMENTED}
"
)
} else {
format!(
"{enter}
def __exit__(self, *args):
def __exit__(self, exc_type: type[BaseException] | None, exc_value: BaseException | None, traceback: TracebackType | None) -> bool | None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the mypy docs, this should be the type signature

@@ -1403,21 +1403,21 @@ class {camel}(Flag):
let docs =
format!(r#""""{newline}{indent}{doc}{newline}{indent}"""{newline}{indent}"#);
let enter = r#"
def __enter__(self):
def __enter__(self) -> Self:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this only works as of 3.11, but should be fine for the component

@@ -1348,14 +1348,14 @@ class {camel}(Flag):
if stub_runtime_calls {
format!(
"
def {snake}({params}):
def {snake}({params}){return_type}:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed these were already generated but not added

OutgoingResponse,
Fields,
OutgoingBody,
OutgoingRequest,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems my autoformatting got me here.. I can revert these changes if you like

@dicej
Copy link
Collaborator

dicej commented Oct 29, 2024

Thanks for doing this, @benbrandt. I haven't had time yet to do more than skim it, but will review it properly tomorrow.

Copy link
Collaborator

@dicej dicej left a comment

Choose a reason for hiding this comment

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

LGTM; thanks! Just one optional suggestion about checking the output in test_examples.sh (see comment below).


# HTTP
# Just compiling for now
(cd examples/http \
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could potentially use curl and nc in background jobs to test the HTTP and TCP examples, respectively. Or else add a test crate written in Rust that exercises them. Not necessary for this PR, though -- just something we could follow up with later.

cargo build --release

# CLI
(cd examples/cli \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could send the output of wasmtime run to a file and then run diff to compare that with another file containing the expected output. Likewise for the matrix-math and sandbox examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding this, I wonder if it would be worth doing a Rust crate for this to make it easier to compare the output as well. I don't think I'll get to it today, but I think it could work nicely to solve both problems and shouldn't be too hard to write.

I'm not sure why I didn't default to this in the first place ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM. If you don't have time for it now, we could go ahead and merge this as-is and add the Rust crate as a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Realistically, I might not get to this until the weekend, so I'd say it is up to you. If I'm lucky I'll get it done tomorrow, but we'll see

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll go ahead and merge this. No hurry on the Rust crate; this is already a big improvement.

@dicej dicej merged commit 707b508 into bytecodealliance:main Oct 31, 2024
3 checks passed
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