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

lib: Supporting building for wasm32-wasip1 #4519

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Earthmark
Copy link
Collaborator

It's really strange not putting a description of changes in this message.

This is my first set of JJ managed changes, I hope this works.

Copy link
Owner

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Thanks. I suppose it would make sense to add this build to CI. Do you want to follow up with that?

Copy link
Owner

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

If anyone here knows more about WebAssembly (more than ~nothing, that is), please review this. Otherwise I think we can merge this in a day or so.

@PhilipMetzger
Copy link
Collaborator

Can you please adjust the commit message to lib: Supporting building for wasm32-wasip1, see https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews. And I'd like to know the motivation behind supporting it, if there are any. And maybe it'd be nice to coordinate the changes with #4478, since that PR is a major rework for lib/src/local_working_copy.rs?

@Earthmark
Copy link
Collaborator Author

Earthmark commented Sep 22, 2024

Oh something went wrong, all the files in the project got modified. I think they got mangled newline wise (my dev setup is a bit strange)

Copy link
Collaborator

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

Seems fine, but let's add a build matrix entry too. I suspect we'll probably need to remove git support for that to work? I would be surprised if libgit2 and the needed deps all build cleanly.

If we could actually build the CLI without git support, we would in theory throw away most of the external deps, and be able to write a WASI backend that used the browser filesystem APIs. Then we could host a version of JJ in the browser as an interactive tutorial. :)

lib/src/local_working_copy.rs Show resolved Hide resolved
@Earthmark Earthmark changed the title Fixed lib building for wasm32-wasip1 lib: Supporting building for wasm32-wasip1 Sep 22, 2024
@Earthmark
Copy link
Collaborator Author

Thanks. I suppose it would make sense to add this build to CI. Do you want to follow up with that?

Step added, it should verify lib builds for wasi.

@Earthmark
Copy link
Collaborator Author

This should probably wait until #4478 goes through, I feel that change is likely a higher precedence and it'd be easy to rebuild these changes.

@Earthmark
Copy link
Collaborator Author

Can you please adjust the commit message to lib: Supporting building for wasm32-wasip1, see https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews. And I'd like to know the motivation behind supporting it, if there are any. And maybe it'd be nice to coordinate the changes with #4478, since that PR is a major rework for lib/src/local_working_copy.rs?

Commit message adjusted.

The use case for this is building out very trivial vfs-wasi support to https://github.com/RobertBaruch/dergwasm to represent entities in the game world as entries in a filesystem, and running jj as a wasm executable to version objects inside the VR environment, from inside the environment itself.

@thoughtpolice 's use case of a web tool on an example filesystem is a much more realizeable use case.

I said this in another comment, but I feel #4478 should probably go through first, as it's a much more complex change.

@Earthmark
Copy link
Collaborator Author

Turns out the executable bit of every file got set on accident.

My main machine is windows, but I worked on this repo in a linux dev container with the folder mounted into the container. I'm pretty sure it exposes mounted windows files as 755, which got committed to the repo by the dev container. In my initial change I ran jj from the windows env, for the follow up I ran it in the container itself, which is why the issue only came up in the follow up.

@martinvonz
Copy link
Owner

Oops :) Sounds like #4478 will be useful for you too then.

This is adapting the FS apis in `lib` for wasi (excluding git support).

The compilation command for building this is:
`cargo +nightly build --no-default-features --target wasm32-wasip1`

* Nightly is needed for `std::os::wasi::fs::symlink_path`, if symlink support isn't needed then this can be non-nightly.
* No default features is needed to not compile gitoxide, as that doesn't yet support wasi.

This does not adapt the CLI, just lib.
@PhilipMetzger
Copy link
Collaborator

Can you please adjust the commit message to lib: Supporting building for wasm32-wasip1, see https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews. And I'd like to know the motivation behind supporting it, if there are any. And maybe it'd be nice to coordinate the changes with #4478, since that PR is a major rework for lib/src/local_working_copy.rs?

Commit message adjusted.

Thanks.

The use case for this is building out very trivial vfs-wasi support to https://github.com/RobertBaruch/dergwasm to represent entities in the game world as entries in a filesystem, and running jj as a wasm executable to version objects inside the VR environment, from inside the environment itself.

That sounds like a really interesting idea, although I think that just having the auto-snapshotting mechanism could be enough for you.

Thank you for enabling the support for it.

@Earthmark
Copy link
Collaborator Author

The ability to restructure changes is very useful as well. I'm happy to talk about it over discord if you're curious!

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