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

feat: add discord link to end, minor tweaks to verbiage #35

Merged
merged 5 commits into from
Oct 12, 2023

Conversation

wikiwong
Copy link
Contributor

No description provided.

README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -18,7 +18,7 @@ Add the library from [crates.io](https://crates.io/crates/extism-pdk).
cargo add extism-pdk
```

Change your `Cargo.toml` to set the crate-type to `cdylib`:
Change your `Cargo.toml` to set the crate-type to `cdylib` (this instructs the compiler to produce a Wasm file):
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't exactly accurate. Compiling to a wasm32-* target is what makes the output a wasm file. This tells the compiler that you are emitting a dynamic library and not an executable (which in wasm parlance means a command pattern module with a _start function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah I can see how that would be misleading. I meant as opposed to a .dylib or .so.

(which in wasm parlance means a command pattern module with a _start function)

I thought the presence of a main function is what determines whether or not it is a command or reactor module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this:

Change your `Cargo.toml` to set the crate-type to `cdylib` (this instructs the compiler to produce a dynamic library, which for our target will be a Wasm file):

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're writing in c or rust your "main" function is called main in the source, but when it turns into wasm it becomes can export called _start

Copy link
Contributor

@bhelx bhelx Oct 11, 2023

Choose a reason for hiding this comment

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

I meant as opposed to a .dylib or .so

That would be determined by the target flag. This is from the wasm-pack doc but relevant:

Here though crate-type = ["cdylib"] typically signifies that you'd like the compiler to create a dynamic system library, but for WebAssembly target it simply means "create a *.wasm file without a start function". On other platforms this output type will create *.so file on Linux, *.dylib on macOS, and *.dll Windows.

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 that definition is in line with my latest update

README.md Outdated
@@ -259,8 +259,7 @@ pub fn hello_from_python() -> FnResult<String> {
### Testing it out

We can't really test this from the Extism CLI as something must provide the implementation. So let's
write out the python side here. You should check the docs for the language you're using on the host side
for how to implement host functions.
write out the Python side here. Check out the [docs for the host SDKs](https://extism.org/docs/category/integrate-into-your-codebase) to implement a host function in a language of your choice.
Copy link
Contributor

Choose a reason for hiding this comment

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

This page will be going away I think. Might be better to link to the Host SDK concept doc?

Copy link
Contributor

@bhelx bhelx left a comment

Choose a reason for hiding this comment

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

I have a couple comments we should fix. Otherwise looks good

@wikiwong wikiwong merged commit 81e7af6 into main Oct 12, 2023
2 checks passed
@wikiwong wikiwong deleted the readme-updates branch October 12, 2023 03:26
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