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(dlang): Add a D host SDK #412

Merged
merged 27 commits into from
Oct 23, 2023
Merged

feat(dlang): Add a D host SDK #412

merged 27 commits into from
Oct 23, 2023

Conversation

chances
Copy link
Contributor

@chances chances commented Aug 10, 2023

See dlang.org.

SDK Coverage: ~91.30% (21/23)

Issues

Todo List

  • Flesh out initial binding
  • Cover host API:
    • ExtismFunctionType
      See FunctionType alias.
    • extism_plugin_id
    • extism_plugin_new_error_free
    • extism_plugin_error
  • Add unit tests
  • Add usage example

@nilslice
Copy link
Member

Wow, @chances - this is awesome! Thank you 🙏 we will review asap and get back to you with any questions.

@zshipko
Copy link
Contributor

zshipko commented Aug 18, 2023

Nice! As far as I can tell this code makes sense, but I don't really know D and didn't see any tests or anything to run. Definitely curious to try it out!

As part of our push toward Extism 1.0 (see #42) we're looking to move the SDKs into their own repos, it might be a good idea to start this off in its own repo to begin with?

@chances
Copy link
Contributor Author

chances commented Aug 26, 2023

Nice! As far as I can tell this code makes sense, but I don't really know D and didn't see any tests or anything to run. Definitely curious to try it out!

...

(Emphasis added)

I totally plan to add unit tests (D lets you define them inline 🎉), but I haven't made time for this step yet. I've added a TODO list to the PR description.

@chances
Copy link
Contributor Author

chances commented Aug 26, 2023

...

As part of our push toward Extism 1.0 (see #42) we're looking to move the SDKs into their own repos, it might be a good idea to start this off in its own repo to begin with?

Is there an extant repo or an example you can point me to so I can try this out without flailing in the dark? What is the procedure for getting a new repo added to the @extism org?

@zshipko
Copy link
Contributor

zshipko commented Aug 26, 2023

...
As part of our push toward Extism 1.0 (see #42) we're looking to move the SDKs into their own repos, it might be a good idea to start this off in its own repo to begin with?

Is there an extant repo or an example you can point me to so I can try this out without flailing in the dark? What is the procedure for getting a new repo added to the @extism org?

We haven't fully discussed that process yet, but I think we could create the extism/d-sdk repo from the start to avoid the "flailing in the dark" issue - soon we'll also be splitting apart the languages into their own repos so it won't feel so isolated at that point.

I'll bring this up at our team meeting on Monday!

@chances
Copy link
Contributor Author

chances commented Sep 1, 2023

@zshipko Were y'all able to discuss the SDKs-in-separate-repos issue this week?

@zshipko
Copy link
Contributor

zshipko commented Sep 1, 2023

Ah yes, sorry!

We decided that since the process of extracting the SDKs into their own repos would happen gradually it would still make sense to merge this into the current repo.

We are also hoping to make all the new SDK repos have a common interface for building/testing. Since this doesn't exist yet we don't want to hold you up!

Also, none of us are D users (yet?) so we will probably need some additional help with understanding D package management and how to setup github actions for testing/releases.

@chances
Copy link
Contributor Author

chances commented Sep 1, 2023

@zshipko Thank you.

D has a standard package manager (Dub) and a common repo of packages: code.dlang.org. Either I or one of your administrators will login to the package repo and register this repo as a D package once the PR is merged.

I am setting up a D continuous integration script as we speak.

@chances
Copy link
Contributor Author

chances commented Sep 1, 2023

The example fails at runtime:

stop reason = EXC_BAD_ACCESS
frame #0: 0x0000000101150a79 libextism.dylib`extism_runtime::manifest::Manifest::new::hd10fbb1ab9120ad8 + 105
frame #1: 0x0000000101162d84 libextism.dylib`extism_runtime::plugin::Plugin::new::hd1dc56981df79447 + 404
frame #2: 0x000000010124aead libextism.dylib`extism_runtime::context::Context::new_plugin::hc510f639ef593a32 + 29
frame #3: 0x00000001011521a7 libextism.dylib`extism_plugin_new + 343
frame #4: 0x000000010001c749 extism_hello`_D6extism6Plugin6__ctorMFNcxAhAPxS3__C14ExtismFunctionbZSQCdQBz(this=0x0000000102800000, withWasi=false, functions=const(runtime.ExtismFunction)*[] @ 0x00007ff7bfefe860, wasm=(length = 18635, ptr = "")) at extism.d:157
frame #5: 0x0000000100002b00 extism_hello`_Dmain at app.d:12

@zshipko
Copy link
Contributor

zshipko commented Sep 1, 2023

When I try to run the example on Linux using dub run extism:hello from the root of the repo I get this error:

    Building package extism:hello in /home/zach/devel/extism/extism/d/examples/hello/
    Starting Performing "debug" build using dmd for x86_64.
    Building extism:hello 0.5.0+commit.27.g98e9ca5: building configuration [application]
Error: unrecognized switch '-P-I/home/zach/devel/extism/extism'
       run `dmd` to print the compiler manual
       run `dmd -man` to open browser on manual
Error dmd failed with exit code 1.

Could I be on the wrong version of dmd or something? I just installed the latest from nix, which is 2.100.2.

@chances
Copy link
Contributor Author

chances commented Sep 21, 2023

@chances this is what I get when I try to test the library:

(dmd-2.105.2)mo@mo-laptop:/mnt/d/dylibso/x/extism$ dub test
             Generating test runner configuration 'extism-test-library' for 'library' (sourceLibrary).
    Starting Performing "unittest" build using /home/mo/dlang/dmd-2.105.2/linux/bin64/dmd for x86_64.
    Building extism 0.5.0+commit.41.gbe7d5d2: building configuration [extism-test-library]
d/extism.d(11,9): Error: undefined identifier `I32`, did you mean enum member `f32`?
d/extism.d(12,9): Error: undefined identifier `I64`, did you mean enum member `f64`?
d/extism.d(13,9): Error: undefined identifier `F32`, did you mean enum member `f32`?
d/extism.d(14,9): Error: undefined identifier `F64`, did you mean enum member `f64`?
d/extism.d(15,10): Error: undefined identifier `V128`, did you mean enum member `v128`?
d/extism.d(16,13): Error: undefined identifier `FuncRef`, did you mean enum member `funcRef`?
d/extism.d(17,15): Error: undefined identifier `ExternRef`, did you mean enum member `externRef`?
d/extism.d(145,17): Error: undefined identifier `ExtismPlugin`
d/extism.d(154,3): Error: undefined identifier `ExtismFunction`
Error /home/mo/dlang/dmd-2.105.2/linux/bin64/dmd failed with exit code 1.

Am I missing something?

These symbols are defined in runtime/extism.h. This sounds like some issue with how d/runtime.c was automatically preprocessed by the D compiler.

This issue does not occur in CI.

@mhmd-azeez What OS are you testing on?

@mhmd-azeez
Copy link
Collaborator

mhmd-azeez commented Sep 21, 2023

@chances

The lint task succeeds in my testing. If you clean *.d files from the targets folder does dub lint succeed?

Yes.

These symbols are defined in runtime/extism.h. This sounds like some issue with how d/runtime.c was automatically preprocessed by the D compiler.

This issue does not occur in CI.

@mhmd-azeez What OS are you testing on?

I am using WSL, probably windows and linux binaries are mixed. Will try it out on pure linux later on

@chances
Copy link
Contributor Author

chances commented Sep 24, 2023

An issue has been raised (dlang/dub#2700) to address the linting issues with intermediate targets.

Copy link
Contributor

@zshipko zshipko left a comment

Choose a reason for hiding this comment

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

These symbols are undefined in the latest release (v0.5.0):

Those symbols aren't available in any release yet, I think it makes sense to target the main branch for now.

I was able to run the example successfully on Linux, and the code looks good!

What do you think is needed for this to be considered ready?

@chances
Copy link
Contributor Author

chances commented Sep 26, 2023

@zshipko

These symbols are undefined in the latest release (v0.5.0):

Those symbols aren't available in any release yet, I think it makes sense to target the main branch for now.

I was able to run the example successfully on Linux, and the code looks good!

I am unable to successfully run the example on my macOS machine because the cargo package fails to compile. Is there a specific version of rust I ought be using?

What do you think is needed for this to be considered ready?

I suppose we can iterate on the unit tests. In lieu of that:

  • Who will be in charge of maintaining the D package in the D package registry? Someone will just need to add the repo to the registry, i.e. https://github.com/extism/extism, and the registry will keep the package updated automatically when new release tags are pushed. We can also include the project's logo. (Where is this asset, by the way?)

    I can do this if you need.

  • How shall we amend the website to include D? For example, on the Integrate into your codebase page?

@zshipko
Copy link
Contributor

zshipko commented Sep 26, 2023

I am unable to successfully run the example on my macOS machine because the cargo package fails to compile. Is there a specific version of rust I ought be using?

stable should work, it looks like the latest wasmtime release requires 1.70.0 (see https://github.com/bytecodealliance/wasmtime/blob/main/RELEASES.md#changed-1)

Who will be in charge of maintaining the D package in the D package registry?

We would definitely appreciate your help with this! I will look into the process a little more.

How shall we amend the website to include D?

We are very close to opening up to docs website for contributions, so if you are up to it we would be happy to have someone familiar with the language help with the docs. But working toward 1.0 we are trying to move all the docs closer to the actual code. So if D has a specific way of handling documentation it would be best to use that for now, especially if it's something that can be checked automatically when running the tests. Since the goal is to move the D code to it's own repo, we could even have an Action to publish the docs if needed.

@chances chances marked this pull request as ready for review September 26, 2023 17:06
@chances
Copy link
Contributor Author

chances commented Sep 26, 2023

@zshipko I have the example working locally after I upgraded my rust installation.

@nilslice
Copy link
Member

nilslice commented Oct 2, 2023

Hi @chances -- sorry for hijacking this PR thread, but I have a tangential question for you!

When doing the FFI work for this SDK, did you happen to reference this page of docs?

https://extism.org/docs/concepts/runtime-apis

Asking as we are in the process of revamping the documentation and considering moving these docs somewhere else.

Thanks!

@chances
Copy link
Contributor Author

chances commented Oct 17, 2023

When doing the FFI work for this SDK, did you happen to reference this page of docs?

https://extism.org/docs/concepts/runtime-apis

Yes, I referenced that page extensively, especially with relation to the doc comments in the D bindings.

@bhelx
Copy link
Contributor

bhelx commented Oct 18, 2023

This is looking good to merge to me. Here is what I suggest we do:

  1. we can merge
  2. we can make a new d-sdk repo and move all the commits to that
  3. we can continue work from there

We do have one change we'd like to get in before doing that as mentioned here: #511, But also i don't want to block this PR any further. @chances, what do you think? Would you be able to help with the breaking change after we merge?

@chances
Copy link
Contributor Author

chances commented Oct 20, 2023

@bhelx Yes, that sounds good to me!

Regarding your question, which breaking change in the linked PR are you referring to? I can certainly help with changes to the D SDK, regardless.

@bhelx
Copy link
Contributor

bhelx commented Oct 23, 2023

Okay i'm gonna merge this today and move it over to d-sdk repo. Will report back when it's done.

@bhelx bhelx merged commit abb31ee into extism:main Oct 23, 2023
3 checks passed
@bhelx
Copy link
Contributor

bhelx commented Oct 23, 2023

Alright, this repo along with commits has been moved to https://github.com/extism/d-sdk
we will remove this copy in extism/extism soon once we get the docs pointing to the right places.

@bhelx
Copy link
Contributor

bhelx commented Oct 23, 2023

@chances Thanks! I'll make an issue on the new repo. I'll also drop any other items I think we'll need for 1.0.

@bhelx
Copy link
Contributor

bhelx commented Oct 23, 2023

BTW, thanks again this is very cool to see! I've created a #d-sdk channel in our Discord as well https://discord.com/channels/1011124058408112148/1166121366920118424

Discord invite: https://discord.gg/q4vJjtbg

@bhelx
Copy link
Contributor

bhelx commented Oct 31, 2023

Just following up here, zach already grabbed the namespace change (the breaking change) we were referring to so I think it's good extism/d-sdk#1

@chances chances deleted the dlang branch November 3, 2023 10:13
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.

None yet

5 participants