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

[examples/raytrace-parallel] Replace Bash with Python for Windows compatibility #3603

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

jthemphill
Copy link
Contributor

@jthemphill jthemphill commented Sep 10, 2023

I wanted to get this example running on my Windows machine without fiddling with Powershell environment variables, so I just rewrote the shell scripts as Python scripts.

Since this example already requires you to have a Python3 installation, this PR makes the example work cross-platform without introducing a new dependency on Python3.

It looks like CI has some dependency on build.sh existing, so I just made it redirect to build.py. That may make any error messages look a bit less friendly in CI, since they'll be Python tracebacks😥 I'll let you decide if this is worth the tradeoff!

@jthemphill jthemphill force-pushed the cross-platform-raytracing branch from d9a8d65 to b90a925 Compare September 10, 2023 04:27
@jthemphill jthemphill changed the title Replace Bash with Python for Windows compatibility [examples/raytrace-parallel] Replace Bash with Python for Windows compatibility Sep 10, 2023
@jthemphill jthemphill force-pushed the cross-platform-raytracing branch 2 times, most recently from 5870d05 to 874d314 Compare September 10, 2023 04:42
@daxpedda daxpedda added the needs review Needs a review by a maintainer. label Sep 11, 2023
@daxpedda
Copy link
Collaborator

I would be fine with it since the server is already in Python. Though I dislike that we are using shell scripts or Python at all here.

Considering all this stuff needs nightly anyway, maybe we could use cargo scripts instead?

Though this LGTM, I'm not familiar with Python, so another maintainer should have a look at this.

@Liamolucko
Copy link
Collaborator

This also seems fine to me.

I think it'd make sense to do the same thing for the wasm-audio-worklet example as well, since it also uses multithreading and as a result needs a basically identical build script and Python server. The same goes for #3606 and #3607.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Thanks!

@daxpedda daxpedda requested a review from Liamolucko September 13, 2023 08:04
Copy link
Collaborator

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

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

wasm-audio-worklet's README should also be updated.

@daxpedda daxpedda added waiting for author Waiting for author to respond and removed needs review Needs a review by a maintainer. labels Sep 27, 2023
@jthemphill jthemphill force-pushed the cross-platform-raytracing branch from 9faf55f to a890159 Compare September 28, 2023 05:56
@jthemphill
Copy link
Contributor Author

Almost all of this was merged in when you merged #3603, because I had rebased #3603 onto this branch. The only things left to merge in this PR now are the README changes. Sorry for not communicating this better😥

@daxpedda daxpedda force-pushed the cross-platform-raytracing branch from a890159 to 69a1e6e Compare September 28, 2023 07:39
@daxpedda daxpedda removed the waiting for author Waiting for author to respond label Sep 28, 2023
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Again, apologies for messing up in #3606.
I rebased this on top of #3606, so all changes should be included now.

@daxpedda daxpedda merged commit f117406 into rustwasm:main Sep 29, 2023
25 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.

3 participants