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

Fix wrong ignore path and create a lot if features #168

Merged
merged 10 commits into from
Jul 24, 2023

Conversation

ASMfreaK
Copy link
Contributor

@ASMfreaK ASMfreaK commented Mar 5, 2023

This PR fixes the issue of wrong import path for the engine. The fix is simple : we put everything needed for the wasm script to run in a single library, hosted on crates.io. (This includes former engine and shared crates).

I've also added a few features for overall more flexible usage:

  1. Added a flags to output different stages of the wasm module build, an option to rename the module, etc.
  2. The only breaking change is in the output naming. The default name of the module is now "tinysearch-implementation".
  3. I've added an ability to use tinysearch as a library in a bigger wasm project directly. There is an example on how to do it in the new examples folder.

@ASMfreaK ASMfreaK marked this pull request as ready for review March 5, 2023 04:31
@mre
Copy link
Member

mre commented Mar 5, 2023

Looks great! I like the the concept of stages; nice idea.

Couple of comments:

  • We need to fix the tests to make the build pass. Right now it fails on include_str!("../assets/demo.html");.
  • Added a comment about THIS_VALUE_SHOULD_BE_FILLED, which I'm a bit confused by
  • What was the reason behind changing the name of the output module? I don't mind too much, but maybe we can rethink that decision if it's just for aesthetical reasons to avoid a breaking change. If we want to go forward with the rename, then we could think about different names as well, such as tinysearch-output.

Thanks a lot for looking into this.

tinysearch/src/main.rs Outdated Show resolved Hide resolved
@ASMfreaK
Copy link
Contributor Author

ASMfreaK commented Mar 5, 2023

* [ ]  We need to fix the tests to make the build pass. Right now it fails on `include_str!("../assets/demo.html");`.

I'll look into this, right now I can't reproduce the issue locally.

* [ ]  Added a comment about `THIS_VALUE_SHOULD_BE_FILLED`, which I'm a bit confused by

I've added a comment here oh GH, I'll add a comment in Cargo.toml template. You see, the file is no longer copied as is, but it is now a template, which is filled out in the generating code.

* [ ]  What was the reason behind changing the name of the output module? I don't mind too much, but maybe we can rethink that decision if it's just for aesthetical reasons to avoid a breaking change. If we want to go forward with the rename, then we could think about different names as well, such as `tinysearch-output`.

The reason is like this: since the merger of two libs (shared and engine) in one crate, I gave it tinysearch-engine name. Therefore the generated crate needed to be renamed to something other, than tinysearch-engine.
I now think, that the shared crate should retain its name - tinysearch-shared. I'll rename it back and bump all the versions to 0.8.0. This way the output could remain named tinysearch-engine.

@mre
Copy link
Member

mre commented Mar 5, 2023

I'll look into this, right now I can't reproduce the issue locally.

Didn't you remove bin/assets/demo.html? I can't see that it was moved in your PR. That would explain the test failure, but not why it doesn't fail locally.

I now think, that the shared crate should retain its name - tinysearch-shared. I'll rename it back and bump all the versions to 0.8.0. This way the output could remain named tinysearch-engine.

Cool!

@ASMfreaK
Copy link
Contributor Author

ASMfreaK commented Mar 5, 2023

Here is a new version.
I've merged lib and bin crates into one - now it's called tinysearch. We should bump at least minor version.
The installation is now done:

cargo install --features=bin tinysearch

I hope, we'll see why it failed earlier with demo.html

@mre
Copy link
Member

mre commented Mar 6, 2023

Guess we also need to update the CI pipelines; specifically the publish-check. Not sure what's up with the build pipeline, but maybe we can ignore that for now. As for publish-check, the paths to the crates need to be updated. Can you have a look?

@ASMfreaK
Copy link
Contributor Author

ASMfreaK commented Mar 7, 2023

It seems that we need to replace actions-rs clippy implementation with another, better maintained one like here .

Signed-off-by: Pavel Pletenev <cpp.create@gmail.com>
@ASMfreaK
Copy link
Contributor Author

Can you please take a look?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@mre
Copy link
Member

mre commented Mar 17, 2023

Thanks for your patience.

There was a typo in the pipeline, which I fixed real quick. Now it fails because the examples folder is missing a Cargo.toml. I've create a little suggestion which should resolve the problem. Can you have a look and maybe apply it so we can give it a spin? Other than that looks good.

Co-authored-by: Matthias Endler <matthias@endler.dev>
@ASMfreaK ASMfreaK changed the title Fix wrong ignore path and create a lot if fetures Fix wrong ignore path and create a lot if features Apr 11, 2023
@mre
Copy link
Member

mre commented Apr 13, 2023

It's failing on nightly. We'll probably have to upgrade the time dependency to fix it.

@Jieiku
Copy link
Collaborator

Jieiku commented Apr 14, 2023

@ASMfreaK I just tried building this from git and then using it on a sample json file unsuccessfully: #151 (comment)

@mre was considering going ahead and merging this and fixing the tests later, so I did a quick manual test of the branch.

@mre mre merged commit 2a89670 into tinysearch:master Jul 24, 2023
5 checks passed
@mre
Copy link
Member

mre commented Jul 24, 2023

@ASMfreaK thanks for your contribution. I made a few more changes and this finally looked good to go, so I merged it. I tested it locally before and didn't run into any issues. @Jieiku fyi.

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.

None yet

3 participants