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

Wasm web support #620

Closed

Conversation

Anderssorby
Copy link

@Anderssorby Anderssorby commented Apr 14, 2022

This adds:

  • A wasm feature
  • The ability to compile without fd-lock which makes it possible to compile to wasm

@gwenn
Copy link
Collaborator

gwenn commented Apr 18, 2022

https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html

If you’re building a non-end product, such as a rust library that other rust packages will depend on, put Cargo.lock in your .gitignore.

@gwenn gwenn added the invalid label Apr 18, 2022
@johnchandlerburnham
Copy link

@gwenn Fixed. Tests pass locally for me, but if you can approve us to run workflows we can go through CI.

Also happy to keep/remove the Nix build from this PR (can move it to a separate PR if its of interest)

@gwenn
Copy link
Collaborator

gwenn commented Apr 20, 2022

nushell/nushell@65008bb ?
And are you sure that nix files are useful for a rust library ?

@johnchandlerburnham
Copy link

Not sure what nushell has to do with it, but there are advantages to having a nix build, such as portability for contributors. However, it also adds some complexity which you might not want.

Happy to keep or remove the nix build, up to you.

@gwenn
Copy link
Collaborator

gwenn commented Apr 20, 2022

And there is a related pending PR:
https://github.com/kkawakam/rustyline/pull/604/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542
where FileHistory can be made optional.

fix .gitignore and Cargo.lock
@Anderssorby Anderssorby marked this pull request as ready for review April 20, 2022 18:43
@Anderssorby
Copy link
Author

I've removed the nix stuff. The PR should be minimal now.

@Anderssorby
Copy link
Author

@gwenn I guess it should be easy to integrate this into your PR?

@gwenn
Copy link
Collaborator

gwenn commented Apr 20, 2022

Except it is redundant with PR #604

@johnchandlerburnham
Copy link

johnchandlerburnham commented Apr 20, 2022

It's not redundant. This PR is a minimal change of 25 lines which gets rustyline building on WASM. #604 is a >700 line change feature-level change which has been open for 2 months and has 2 of 5 tasks completed. It seems like merging this PR should be much easier, which will then let us use rustyline for our web repl: argumentcomputer/lurk-beta#57

That said, if you guys prefer to wait for #604 to have wasm support, that's fine with us, and we can just fork off a separate crate for our purposes in the meantime.

Cargo.toml Outdated Show resolved Hide resolved
src/history.rs Outdated Show resolved Hide resolved
src/history.rs Outdated Show resolved Hide resolved
@gwenn
Copy link
Collaborator

gwenn commented Apr 21, 2022

@Anderssorby
Copy link
Author

@gwenn I've fixed the comments and warnings. Let me know if there is anything else you want to change.

@Anderssorby Anderssorby requested a review from gwenn April 21, 2022 14:37
@Anderssorby
Copy link
Author

@gwenn Any updates on this? It would be awkward to have to crate a new crate just to fix wasm compilation...

@gwenn
Copy link
Collaborator

gwenn commented Apr 25, 2022

I am wondering why not using #[cfg(not(target_arch = "wasm32"))] / #[cfg(target_arch = "wasm32")] ?

@Anderssorby
Copy link
Author

I feel this is more explicit and meaningful. Maybe someone adds wasm (with wasi) support for this? Also one can now explicitly disable the fd-lock dependency for other cases too.

@gwenn
Copy link
Collaborator

gwenn commented Apr 26, 2022

Also one can now explicitly disable the fd-lock dependency for other cases too.

You mean like in PR #604 !

mhuesch added a commit to neighbour-hoods/rep_lang that referenced this pull request Jun 21, 2022
- depend on kkawakam/rustyline#620 PR code
- fork PR branch s.t. it can't disappear on us
mhuesch added a commit to neighbour-hoods/rep_lang that referenced this pull request Jun 21, 2022
- depend on kkawakam/rustyline#620 PR code
- fork PR branch s.t. it can't disappear on us
mhuesch added a commit to neighbour-hoods/rep_lang that referenced this pull request Jun 21, 2022
work around lack of proper wasm support in rustyline. rustyline changed
since we updated deps in #73, so wasm builds broke.

solution:
- depend on kkawakam/rustyline#620 PR code
- fork PR branch s.t. it can't disappear on us
@gwenn gwenn added duplicate and removed invalid labels Jan 15, 2023
@samuelburnham
Copy link

Now that #604 is merged, rustyline compiles to wasm32-unknown-unknown on the master branch when the with-file-history feature is disabled. Thanks @gwenn! There are still a few warnings about unused variables/imports, but I think it's safe to close the PR.

@gwenn gwenn closed this Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants