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

Rewrite from proc-macro to build.rs #15

Merged
merged 16 commits into from
Nov 11, 2024
Merged

Rewrite from proc-macro to build.rs #15

merged 16 commits into from
Nov 11, 2024

Conversation

marlonbaeten
Copy link
Member

@marlonbaeten marlonbaeten commented Oct 10, 2024

The current implementation uses a proc-macro. This macro is not expanded in incremental builds. This causes changes in the static asset directory to not show up in the binary.

This proposed change drops the load_assets! macro and introduces a build script for the memory-serve crate. The build script takes an environment variable to load assets from. Any changes in the path defined by ASSET_DIR or changes to the variable itself will trigger a rerun and will load fresh assets.

Note that this will be a breaking change:

MemoryServe::new(load_assets!("static"))

Becomes:

MemoryServe::from_env()

Update: I have decided to both support the proc-macro and a new method using a build.rs and MemoryServe::from_env().

And the ASSET_DIR environment variable will be required.

This PR also ships some improvements to logging and improves build performance.

Fixes #16

README.md Outdated Show resolved Hide resolved
memory-serve/build.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@paul-hansen paul-hansen left a comment

Choose a reason for hiding this comment

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

I appreciate this change, it's been common to need to manually convince it to rebuild so this is very welcome.

Is it possible to provide ASSET_PATH in some way that I can store in my project's repo? From my understanding, my own build scripts run after dependencies are compiled so I can't set the env var there, and my .env is parsed at runtime so I can't use that.
Just a minor annoyance to have setting that env var in the instructions for building my app when it's going to be the same relative path every time.
Maybe we could optionally parse a variable from the project manifest? (not sure I like more build time deps for parsing though.) Or maybe just provide a function that users should call in their own build script?

Feel free to spin this off into future work if you want to get this out now.

README.md Outdated
Comment on lines 36 to 39
Provide a relative path to the directory containing your static assets
to the [`load_assets!`] macro. This macro creates a data structure intended to
using the `ASSET_PATH` environment variable. This macro creates a data structure intended to
be consumed by [`MemoryServe::new`]. Calling [`MemoryServe::into_router()`] on
the resulting instance produces a axum
Copy link
Contributor

@paul-hansen paul-hansen Oct 21, 2024

Choose a reason for hiding this comment

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

Comment still mentions the removed macro.

Feel free to write something different but I adapted part of the description of the PR to maybe save you some time:

Suggested change
Provide a relative path to the directory containing your static assets
to the [`load_assets!`] macro. This macro creates a data structure intended to
using the `ASSET_PATH` environment variable. This macro creates a data structure intended to
be consumed by [`MemoryServe::new`]. Calling [`MemoryServe::into_router()`] on
the resulting instance produces a axum
Provide a relative path to the directory containing your static assets
using the `ASSET_PATH` environment variable. This path will be used in a build script and any changes to files in that path or the variable itself will trigger a rerun and will load fresh assets. The output of the build script is consumed by [`MemoryServe::new`]. Calling [`MemoryServe::into_router()`] on
the resulting instance produces a axum

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this! I took your suggestion and also mentioned the build script instead of the macro further on. See 7fad903

@marlonbaeten
Copy link
Member Author

@paul-hansen I could also allow configuring the ASSET_DIR in the package metadata of Cargo.toml: https://doc.rust-lang.org/cargo/reference/manifest.html#the-metadata-table Would that work for you?

@paul-hansen
Copy link
Contributor

That'd be great! We already use similar metadata for cargo-leptos so that would be very familiar.

@marlonbaeten
Copy link
Member Author

Hmm - I am sill not happy about the fact that we need to specify an absolute path with the ASSET_DIR variable (or via metadata). I like to find a solution before merging this work.

@paul-hansen
Copy link
Contributor

Hmm - I am sill not happy about the fact that we need to specify an absolute path with the ASSET_DIR variable (or via metadata). I like to find a solution before merging this work.

Looks like rust-embed uses a proc macro but probably avoids the stale assets in incremental builds problem by serving the files from disk in debug mode by default (configurable with a feature). Not sure if that's a good option here, just throwing around ideas.

@marlonbaeten marlonbaeten merged commit 1023a64 into main Nov 11, 2024
8 checks passed
@marlonbaeten marlonbaeten deleted the no-proc-macro branch November 11, 2024 14:06
@praseodym
Copy link

Hmm - I am sill not happy about the fact that we need to specify an absolute path with the ASSET_DIR variable (or via metadata). I like to find a solution before merging this work.

Looks like rust-embed uses a proc macro but probably avoids the stale assets in incremental builds problem by serving the files from disk in debug mode by default (configurable with a feature). Not sure if that's a good option here, just throwing around ideas.

This is how memory-serve already worked, unless the new force-embed feature is enabled.

@marlonbaeten
Copy link
Member Author

marlonbaeten commented Nov 11, 2024

That'd be great! We already use similar metadata for cargo-leptos so that would be very familiar.

Unfortunately the crate metadata is not available when build.rs is executed. So for now there are two options: use the macro as before or use a environment variable. Both work with relative or absolute paths.

@paul-hansen
Copy link
Contributor

paul-hansen commented Nov 11, 2024

So for now there are two options: use the macro as before or use a environment variable. Both work with relative or absolute paths.

Looks like we now use include_bytes! (which should automatically handle incremental reloads) so it should work with incremental builds in either case. Am I understanding that right?

I'm not sure if it works with the procmacro with compression enabled though since we create the compressed file in the procmacro so I think it wouldn't see the change. Maybe when compression is enabled we could still use include_bytes! on the original file just to ensure it's tracked but not actually use it? Looks like that's how rust-embed's include_flate! dependency fixed that: SOF3/include-flate@4cf6b8e Guessing since it's just included when the procmacro runs, so it doesn't get included in the final binary? Not sure though. This issue is what led me to that: pyrossh/rust-embed#182

Edit: I see some problems so I doubt this would work exactly as I described it but maybe someone can adapt it or figure out how rust embed does it from what I linked.

Edit 2: Just trying to be helpful, feel free to ignore as it's good enough as is for my use case. Thanks!

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.

Support for multiple asset directories
4 participants