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

Basic WebAssembly support with minimal Javascript API #615 #1986

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

rujialiu
Copy link

@rujialiu rujialiu commented Sep 2, 2023

Currently 6/12 crypto functions supported. Tested in browser with all default features disabled.

@triska
Copy link
Contributor

triska commented Sep 2, 2023

Very impressive, thank you a lot!

Compilation for WASM seems worth mentioning in the README.md, maybe as a sub-section in https://github.com/mthom/scryer-prolog#installing-scryer-prolog ?

@rujialiu
Copy link
Author

rujialiu commented Sep 2, 2023

Ok! I'll try. Also I noticed that section about native installing on windows is a bit outdated because now a simple Command Line Prompt can handle everything, no need to install msys2. I'm not sure if we need a C/C++ compiler because even ring doesn't require one on windows (precompiled obj is bundled), but since I've already installed VS2022, I'm not quire sure.

Also, I have an impression that to do serious constraint programming with prolog on the web, swi-prolog was the only choice (I don't think tau-prolog supports clpz but I'm not sure trella prolog supports it, if compiled to wasm, and I don't think eclipse can be compiled to wasm without much work), if that's true, scryer-prolog on the web should be really attractive.

@rujialiu
Copy link
Author

rujialiu commented Sep 4, 2023

Now README.md is updated. Can this be merged soon? Since I'll probably be unable to modify this within a week or so... very busy with a tricky large-scale C++ refactor :-S

README.md Outdated
@@ -145,6 +145,88 @@ It will generate a very basic MSI file which installs the main executable and a

Scryer Prolog must be built with **Rust 1.63 and up**.

### Building WebAssembly

Scryer Prolog has basic WebAssembly support. However, none of the default features are currently supported. You can manually change the default features in `Cargo.toml` from
Copy link
Contributor

@Skgland Skgland Sep 4, 2023

Choose a reason for hiding this comment

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

No need to edit Cargo.toml --no-default-features command line flag or default-features = false in [dependencies] should be sufficient

Copy link
Contributor

Choose a reason for hiding this comment

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

Should work with wasm-pack build as an extra option.

Copy link
Author

Choose a reason for hiding this comment

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

Should work with wasm-pack build as an extra option.

I agree that passing --no-default-features to wasm-pack should theoretically work and is the recommended way. I even mentioned it in my other PR #1966.

However, it doesn't work on my PC (windows 10). I don't know why. wasm-pack seemed to ignore all extra parameters including --offline. I had to do a regular cargo build --offline and let wasm-pack "remember" I was using --offline last time. And I feel bad to write something in README.md that doesn't work for myself...

After some struggle I wrote down these silly instructions that at least should work for everyone. Anyway, writing the recommended way should be better. Do you think I should keep the workaround in case anyone else is facing the same problem, or remove it for now until I find the real cause on my side? I couldn't find any similar issue on github or stackoverflow so probably I'm the only one having this issue? 8-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rujialiu: Personally, I think the instructions @Skgland posted seem preferable because they do not require any changes in existing files. If these instructions should work, but do not work for you, then could you please consider filing this as an issue in the wasm-pack repository so that others will also benefit from the correction? Thank you a lot!

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to add: I think none of this is any reason to delay merging this PR, because we can easily tweak the instructions at any time! To me, it seems preferable to merge this work as soon as possible so that other PRs can already take it into account.

Copy link
Author

Choose a reason for hiding this comment

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

REDAME.md updated again, thanks @Skgland and @triska for suggestions :) And I agree to merge this asap too!

@triska
Copy link
Contributor

triska commented Sep 5, 2023

Why are the CI tests now suddenly failing? Is this due to conflicting changes that have since been merged?

@Skgland
Copy link
Contributor

Skgland commented Sep 5, 2023

Why are the CI tests now suddenly failing? Is this due to conflicting changes that have since been merged?

Looks to me like the CI runners had Network issues

@triska
Copy link
Contributor

triska commented Sep 7, 2023

@Skgland: How can we trigger the CI tests again? I think everything works as intended with these changes, but the failed tests make it appear as if there were still a problem with them!

@Skgland
Copy link
Contributor

Skgland commented Sep 7, 2023

https://docs.github.com/en/actions/managing-workflow-runs/re-running-workflows-and-jobs should explain this though I don't know if anyone besides @mthom has sufficient access rights (write access) to do it.

@Skgland
Copy link
Contributor

Skgland commented Sep 7, 2023

Alternatively one could push a new commit to trigger a new CI run. If there are no further change to incorporate into an additional commit, one could for example rebase on master and then force push with lease the rebased commits.

@triska
Copy link
Contributor

triska commented Sep 7, 2023

@rujialiu: Could you please consider doing this, i.e., rebasing, so that this PR is clearly visible as passing all CI tests? This may be a good opportunity to also apply two small typographic corrections to the README changes:

  • line 154: "exmapl" → "example"
  • line 169: "your can test" → "you can test"

The PR looks great in all other respects. Thank you a lot!

@rujialiu
Copy link
Author

rujialiu commented Sep 8, 2023

@rujialiu: Could you please consider doing this, i.e., rebasing, so that this PR is clearly visible as passing all CI tests? This may be a good opportunity to also apply two small typographic corrections to the README changes:

Ok! I'll do this when I have some time. I need to look at how to resolve conflicts first.

…ently 6/12 crypto functions supported. Tested in browser with all default features disabled.
@rujialiu
Copy link
Author

rujialiu commented Sep 8, 2023

Done. It turns out that the confusing conflicts are from cargo fmt, so manually re-applying my changes on top of master is easier than "fixing" the conflicts. Hope the CI will work this time!

@mthom mthom merged commit 920e0c6 into mthom:master Sep 8, 2023
@triska
Copy link
Contributor

triska commented Sep 8, 2023

Aaaaaw yes, please consider announcing this in the discussions, and thank you so much @rujialiu !!!

@rujialiu rujialiu deleted the wasm32-support branch September 9, 2023 02:07
@rujialiu
Copy link
Author

rujialiu commented Sep 9, 2023

Aaaaaw yes, please consider announcing this in the discussions, and thank you so much @rujialiu !!!

Ok! Will do it when I have some time!

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.

4 participants