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

add Zig support #144 #196

Merged
merged 29 commits into from
Sep 5, 2023
Merged

Conversation

voigt
Copy link
Contributor

@voigt voigt commented Aug 17, 2023

This PR adds a Zig library to write Workers for wasm-worker-server in Zig (#144) .

Fixes: #144

Some remarks:

  • tested with Zig version 0.11.0
  • still new to Zig, so possible that things can be done more idiomatic/elegant :)
  • I wanted to use Request and Response from Zigs standard library. Unfortunately std.http.Server.Response as well as std.http.Client.Request are not sufficient :( Using std.http.Server.Response even lead to a WASI compile error error: root struct of file 'os.wasi' has no member named 'sockaddr'. Therefore I created custom entities and hope that we can fix this in the future
  • The environment variable example is currently not working. Either I'm doing something wrong or Zigs WASI implementation for std.os.getenv() does not work properly/differently than expected.

Work to be done before merge:

  • Detect the content in the response. If it's a valid UTF-8 string, we can return. If it uses a different encoding, you need to encode it in base64. (I ran into a weird error)...
  • testing of examples
  • testing of docs

@Angelmmiguel
Copy link
Contributor

I saw you already included the documentation! That's amazing @voigt 😄

@ereslibre
Copy link
Contributor

Amazing contribution, thanks @voigt! Although I don't have experience with Zig, I expect to be able to have a look at this PR this week :)

@Angelmmiguel Angelmmiguel requested a review from a team August 24, 2023 08:26
@Angelmmiguel Angelmmiguel added 🚀 enhancement New feature or request 🔨 sdks Issues related to language SDKs labels Aug 24, 2023
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Thanks for the amazing and important contribution @voigt! I think this is already in a very nice shape :)

Disclaimer: despite I like Zig's ideas I don't have experience with it, so I cannot help that much on Zig's specifics. :)

Some comments from my side on a first pass. Thanks again! 👏

README.md Outdated Show resolved Hide resolved
docs/docs/languages/zig.md Outdated Show resolved Hide resolved
docs/docs/languages/zig.md Outdated Show resolved Hide resolved
docs/docs/languages/zig.md Outdated Show resolved Hide resolved
docs/docs/languages/zig.md Show resolved Hide resolved
kits/zig/worker/src/worker.zig Outdated Show resolved Hide resolved
kits/zig/worker/src/worker.zig Outdated Show resolved Hide resolved
kits/zig/worker/src/worker.zig Show resolved Hide resolved
kits/zig/worker/src/worker.zig Outdated Show resolved Hide resolved
kits/zig/worker/src/worker.zig Show resolved Hide resolved
Copy link
Contributor

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

This is an amazing contribution @voigt! I tested the different examples in my laptop and I could run my first Zig workers!!

$ wws zig-out/bin/ 

⚙️  Preparing the project from: zig-out/bin/
⚙️  Loading routes from: zig-out/bin/
⏳ Loading workers from 1 routes...
✅ Workers loaded in 46.583667ms.
    - http://127.0.0.1:8080/basic
      => zig-out/bin/basic.wasm
🚀 Start serving requests at http://127.0.0.1:8080

[2023-08-28T07:58:55Z INFO  actix_web::middleware::logger] 127.0.0.1 "GET /basic HTTP/1.1" 200 682 "-" "..." 0.003404

As you mentioned, environment variables are not working. It would be great to dig into this issue to understand if this is a Zig bug. We can create a separate issue for it, but for now I would delete the zig-envs example from the PR.

I added some minor comments. For me, the only important topic to clarify is the input read process and \n.

Again thank you! I'm really glad to see Zig support on wws 😄

docs/docs/features/environment-variables.md Show resolved Hide resolved
docs/docs/features/http-requests.md Show resolved Hide resolved
kits/zig/worker/src/worker.zig Outdated Show resolved Hide resolved
kits/zig/worker/src/worker.zig Show resolved Hide resolved
kits/zig/worker/src/worker.zig Outdated Show resolved Hide resolved
kits/zig/worker/src/worker.zig Outdated Show resolved Hide resolved
@voigt
Copy link
Contributor Author

voigt commented Aug 28, 2023

Thank you for your feedback @Angelmmiguel & @ereslibre! I will incorporate it asap, during the course of this week :)

Copy link
Contributor

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

Amazing contribution @voigt! I'm really happy to see Zig on Wasm Workers Server! We definitely should write an article about it 😉

I added a last minor comment on the zig-basic example.

examples/zig-basic/src/basic.zig Outdated Show resolved Hide resolved
@voigt voigt marked this pull request as ready for review September 1, 2023 08:13
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Huge LGTM, thanks @voigt for your efforts!

Tried all the examples and they worked perfectly 🎉

@Angelmmiguel
Copy link
Contributor

This is really cool!! 👏 👏 Thank you very much for your contribution @voigt!

Related CLA: #210 (comment)

@Angelmmiguel Angelmmiguel merged commit cb381a3 into vmware-labs:main Sep 5, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request 🔨 sdks Issues related to language SDKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Zig
3 participants