feat: embed webui assets into the binary #385

2e3s · 2023-05-31T01:56:38Z

Hi!

When I was playing with the server, I noticed the inconvenience to distribute. I was able to overcome some problems with things like cargo-deb and manifests, but it's limited to deb. There is also a Tauri subproject but it is very bulky.

The proposed change takes the content of the root's aw-webui/dist and embeds into the aw-server-rust binary. This can make the distribution much easier, and less prone to problems with filesystem locations, and maybe even more secure as the served files cannot be changed anymore. The executable grows only a bit in disk space, but not RAM.

The trait and macros allow to continue using lib.rs easily and flexibly. It uses this crate: https://github.com/pyrossh/rust-embed

What do you think about this idea?

ErikBjare · 2023-06-07T05:55:05Z

I really like this!

And awesome work on awatcher btw!

A few things:

What happens if the web UI is missing during build?
Would be nice if one could easily let it fallback to whatever is in a dir (being able to rebuild the webui without having to restart aw-server-rust is useful to me)

What do you think @johan-bjareholt? :)

2e3s · 2023-06-07T15:48:52Z

What happens if the web UI is missing during build?

The build would fail because the macros requires the specified path to exist compile-time, even an empty directory. Is this a normal scenario during a compilation though? If so, there could be build.rs with std::fs::create_dir_all("aw-webui/dist").

Would be nice if one could easily let it fallback to whatever is in a dir

It could reuse webpath CLI setting: if it's specified during the run, then use the filesystem, otherwise use the embedded files. dirs::get_asset_path() would be unused at all.
It could be vice versa: if a file in the data directory is absent, or if the data directory doesn't exist at all, use the embedded file. It would use webpath CLI setting or dirs::get_asset_path().
Have a default cargo feature "embed" and embed files if the feature is on (cargo build --features=embed). If it is off then work with filesystem. The feature would probably turn on different AssetResolver implementations, removing or leaving a conditional webpath CLI argument like this.

Is your suggestion only for testing aw-webui with the server? Maybe the separate feature or something like that would target your use case more precisely while reducing UX complexuty, because it would remove otherwise useless webpath CLI setting (which I've forgotten about).

2e3s · 2023-06-21T05:29:41Z

It could reuse webpath CLI setting: if it's specified during the run, then use the filesystem, otherwise use the embedded files. dirs::get_asset_path() would be unused at all.

I have implemented this for these reasons:

This option removes some complexity of 100 lines of code.
If data is embedded into the binary then it seems that normally there is no need to try and search for the data throughout the disk, unless you're trying to do something unusual (which is why there is a CLI option).

Please, correct if I'm wrong in my assumptions.

ErikBjare · 2023-09-12T09:32:47Z

Going to try and merge this today.

Not sure why CI isn't running, as with #396. Excuse me for opening a new one to get CI to run.

ErikBjare · 2023-09-12T09:43:16Z

Now checks are running, some kinks to iron out: #420

2e3s · 2023-09-12T23:36:48Z

Thanks, I'll fix this soon 👌

codecov · 2023-09-13T12:32:27Z

Codecov Report

Patch coverage: 50.00% and project coverage change: +1.20% 🎉

Comparison is base (f962ccd) 72.16% compared to head (90951c7) 73.37%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   72.16%   73.37%   +1.20%     
==========================================
  Files          45       45              
  Lines        2831     2794      -37     
==========================================
+ Hits         2043     2050       +7     
+ Misses        788      744      -44

Files Changed	Coverage Δ
aw-server/src/dirs.rs	`80.76% <ø> (+49.88%)`	⬆️
aw-server/src/endpoints/hostcheck.rs	`94.11% <ø> (ø)`
aw-server/src/main.rs	`0.00% <0.00%> (ø)`
aw-server/src/endpoints/mod.rs	`83.95% <54.16%> (+2.61%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

2e3s · 2023-09-13T12:43:49Z

CI is fixed with build.rs, another option is to have aw-webui/dist/.gitkeep in the other repository and load the submodule before testing or building.

The important consequence of this would be not forgetting to have aw-webui compiled before building the complete rust server. As I understand, it should be already ok now in the build script https://github.com/ActivityWatch/activitywatch/blob/master/.github/workflows/build.yml

johan-bjareholt

The executable grows only a bit in disk space, but not RAM.

This doesn't sound right. How did you measure this? Did you only start the application or did you let it run for a while, using the web-ui etc?
If you only start the application and don't use the web-ui, it will likely only use the same amount of memory that it used before because that's all memory will be required to map. However, if you use all assets bundled in the executable, it will be required to map that memory as well. Exactly when the OS will unmap that memory is hard to tell, but most likely it will hold on to that mapped memory for quite some time, likely until the OS runs low on memory.

Regardless, it's probably not a huge issue since the web-ui is only a few MB right now, but if that ever grows we will need to keep that in mind. The pros probably outweigh the cons. Could also be optimized in the future by bundling compressed versions of the files, which then get uncompressed on demand.

aw-server/src/endpoints/mod.rs

johan-bjareholt · 2023-09-16T09:27:47Z

aw-server/src/endpoints/mod.rs

+}
+
+#[macro_export]
+macro_rules! embed_asset_resolver {


Why is it necessary to have a macro for this?
Can't you instead have a single struct that includes the whole "dist" folder?

If __ProjectAssetResolver is not behind the macros, then rust-embed's own derive macros always runs compile-time, which requires always having ../aw-webui/dist relative to this rs file regardless the dynamic dispatch by trait.

Regarding the overall abstraction instead of always embedding ../aw-webui/dist. I don't know about the strategy of this server development, hence I've made the more conservative, least prohibitive choice to allow more freedom and easier use of the server as a crate library, like in the some past instances

https://github.com/ActivityWatch/aw-tauri/blob/master/src-tauri/Cargo.toml#L19 (where I found out about the crate's lib)

https://github.com/2e3s/awatcher/blob/main/Cargo.toml#L39 (implemented a lighter approach than Tauri)

Another way I thought about is interpolate-folder-path feature on, which allows #[folder = "$CARGO_MANIFEST_DIR/aw-webui/dist"] and hence this directory structure can be replicated elsewhere. I had disregarded it because it's not a maximized flexibility.

Another way I thought about is interpolate-folder-path feature on, which allows #[folder = "$CARGO_MANIFEST_DIR/aw-webui/dist"] and hence this directory structure can be replicated elsewhere. I had disregarded it because it's not a maximized flexibility.

That's a pretty cool idea.

A variant on that would be to have a "AW_WEBUI_DIR" variable and if the variable is unset you can set it to "../aw-webui/dist" in build.rs.

My initial suggestion with interpolate-folder-path turned out looking pretty awkward when implemented, but your improvement has made it acceptable. 👍

2e3s · 2023-09-17T01:48:45Z

Did you only start the application or did you let it run for a while, using the web-ui etc?

You're right, I only meant that it doesn't load the data on the application start.
The declaration of embedded data looks like const BYTES: &'static [u8] = include_bytes!() so all the memory nuances are the same to include_bytes!. It really means sitting in a virtual memory and loading into RAM when requested.

There is a compression option for the crate, but it doesn't affect RAM consumption, only the size on disk.

johan-bjareholt · 2023-09-17T19:50:47Z

You're right, I only meant that it doesn't load the data on the application start.
The declaration of embedded data looks like const BYTES: &'static [u8] = include_bytes!() so all the memory nuances are the same to include_bytes!. It really means sitting in a virtual memory and loading into RAM when requested.

There is a compression option for the crate, but it doesn't affect RAM consumption, only the size on disk

I think it's fine as is for now, no need to dig deeper into this right now.

ErikBjare

LGTM, let me know when you feel it is ready to merge @2e3s! :)

2e3s · 2023-09-18T19:02:10Z

I have re-reviewed the code and found a minor bug/typo. Otherwise it's good to go.

2e3s · 2023-09-20T19:13:49Z

let me know when you feel it is ready to merge

Just in case, it's fixed and ready I think 🙂

ErikBjare · 2023-09-24T15:38:41Z

Awesome, merging! 🚀

ErikBjare · 2023-09-24T15:45:12Z

Massive thanks for getting this together @2e3s! 🏅

This is a great QoL improvement, and now that I think about it will massively simplify some parts of aw-android (when we get it building again...) where we before had to unzip the webui and make a mess. Huge win right there, that you didn't even know about!

If you want to get paid, just send me the results from the script.

And thank you @johan-bjareholt for the thorough review!

ErikBjare · 2023-09-26T07:53:44Z

Made the following fixes post-merge as I was working on updating aw-android:

Really improved the situation on Android, much faster startup speed and less complexity!

2e3s added 3 commits May 28, 2023 23:05

Embed dist to the binary

cdb1901

Make asset-resolver into a subcrate

c82c26c

Use macros to embed the asset resolver

1c23a55

Use a site data dir if provided by CLI

14b3476

ErikBjare mentioned this pull request Sep 12, 2023

Embed dist to the binary [ci run] #420

Closed

2e3s added 4 commits September 13, 2023 07:29

Merge branch 'master' into embed-webui

bfdda78

Merge master branch

279fd1f

Upgrade rust-embed

b04b448

Add build.rs

0d38246

johan-bjareholt reviewed Sep 16, 2023

View reviewed changes

2e3s added 2 commits September 17, 2023 21:24

Replace macros to embed by env var

307d352

Remove trait and pass struct directly

da9b87c

ErikBjare approved these changes Sep 18, 2023

View reviewed changes

johan-bjareholt approved these changes Sep 18, 2023

View reviewed changes

Fix asset resolver with file path

90951c7

ErikBjare changed the title ~~Embed dist to the binary~~ feat: embed webui assets into the binary Sep 24, 2023

ErikBjare merged commit 26acc28 into ActivityWatch:master Sep 24, 2023
6 of 7 checks passed

2e3s deleted the embed-webui branch September 24, 2023 18:08

2e3s mentioned this pull request Sep 27, 2023

fix: improve AW_WEBUI_DIR handling #424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: embed webui assets into the binary #385

feat: embed webui assets into the binary #385

2e3s commented May 31, 2023

ErikBjare commented Jun 7, 2023

2e3s commented Jun 7, 2023

2e3s commented Jun 21, 2023 •

edited

Loading

ErikBjare commented Sep 12, 2023

ErikBjare commented Sep 12, 2023

2e3s commented Sep 12, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

2e3s commented Sep 13, 2023 •

edited

Loading

johan-bjareholt left a comment

johan-bjareholt Sep 16, 2023

2e3s Sep 16, 2023 •

edited

Loading

2e3s Sep 17, 2023

johan-bjareholt Sep 17, 2023

2e3s Sep 18, 2023

2e3s commented Sep 17, 2023

johan-bjareholt commented Sep 17, 2023

ErikBjare left a comment

2e3s commented Sep 18, 2023

2e3s commented Sep 20, 2023

ErikBjare commented Sep 24, 2023

ErikBjare commented Sep 24, 2023 •

edited

Loading

ErikBjare commented Sep 26, 2023

feat: embed webui assets into the binary #385

feat: embed webui assets into the binary #385

Conversation

2e3s commented May 31, 2023

ErikBjare commented Jun 7, 2023

2e3s commented Jun 7, 2023

2e3s commented Jun 21, 2023 • edited Loading

ErikBjare commented Sep 12, 2023

ErikBjare commented Sep 12, 2023

2e3s commented Sep 12, 2023

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

2e3s commented Sep 13, 2023 • edited Loading

johan-bjareholt left a comment

Choose a reason for hiding this comment

johan-bjareholt Sep 16, 2023

Choose a reason for hiding this comment

2e3s Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

2e3s Sep 17, 2023

Choose a reason for hiding this comment

johan-bjareholt Sep 17, 2023

Choose a reason for hiding this comment

2e3s Sep 18, 2023

Choose a reason for hiding this comment

2e3s commented Sep 17, 2023

johan-bjareholt commented Sep 17, 2023

ErikBjare left a comment

Choose a reason for hiding this comment

2e3s commented Sep 18, 2023

2e3s commented Sep 20, 2023

ErikBjare commented Sep 24, 2023

ErikBjare commented Sep 24, 2023 • edited Loading

ErikBjare commented Sep 26, 2023

2e3s commented Jun 21, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

2e3s commented Sep 13, 2023 •

edited

Loading

2e3s Sep 16, 2023 •

edited

Loading

ErikBjare commented Sep 24, 2023 •

edited

Loading