search

framesurge / perseus

A state-driven web development framework for Rust with full support for server-side rendering and static generation.
https://framesurge.sh/perseus/en-US
MIT License
2.18k stars 89 forks source link

Dockerfile refactor #153

Closed allen-woods closed 1 year ago

allen-woods commented 2 years ago

Fixed multiple issues with Dockerfile code blocks in docs, as follows:

allen-woods commented 2 years ago

This looks great to me, thanks very much! I'm not on a machine on which I can easily test these right now, and the CI unfortunately doesn't have any testing for them yet, so I assume they run on your machine (in which case they should, given it's Docker, run everywhere)?

There are some unforeseen problems I'm fixing to get this finalized.

Forgive me if I'm wrong, but from what I understand currently, the missing wasm-bindgen dependencies imply that all builds based on current Docker Deployment docs will fail.

Successful compilation via perseus deploy has been achieved for 0.3.5 and later using the following:

Update: ✨ The server is working! ✨

Currently, version 0.3.3 is failing to build due to an error outside the scope of the Dockerfile.

The errors I'm seeing are related to app::get_plugins, something I am unable to locate in the source code used by the container.

arctic-hen7 commented 2 years ago

So everything is working now except for the v0.3.3 one? Could it be pulling in a .perseus/ folder from a newer version? Maybe run perseus clean just to check... If that doesn't work and you've got no other ideas then I'm happy to leave the v0.3.3 docs as they were.

phaleth commented 2 years ago

I'm glad to see this moving forward! I like the changes made by @allen-woods. Been wanting to do an update PR myself, but too short on time lately.

Popping in to say there is this little gotcha that at some point, I think at version 0.3.4 the tiny example dir has moved from examples/tiny to examples/comprehensive/tiny.

Hope this is gonna be merged. I'm learning from the changes proposed. Thank you.

allen-woods commented 2 years ago

@arctic-hen7 So everything is working now except for the v0.3.3 one?

Correct.

@arctic-hen7 Could it be pulling in a .perseus/ folder from a newer version? Maybe run perseus clean just to check... If that doesn't work and you've got no other ideas then I'm happy to leave the v0.3.3 docs as they were.

I will be testing all variations of the Dockerfile code today after I've made my adjustments to all of them, then I'll circle back to the 0.3.3 image in hopes of correcting what may be happening there.

@phaleth I'm glad to see this moving forward! I like the changes made by @allen-woods. Been wanting to do an update PR myself, but too short on time lately.

Thank you, I was hoping I wasn't stepping on any toes by doing a refactor to your work.

@phaleth Popping in to say there is this little gotcha that at some point, I think at version 0.3.4 the tiny example dir has moved from examples/tiny to examples/comprehensive/tiny.

Thanks for the catch (I found it after a couple of failed builds). The manual wee_alloc version of 0.3.5 container compiles successfully, but docker run gives me the following error:

thread 'main' panicked at 'you must provide your own error pages in production', /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/perseus-0.3.5/src/error_pages.rs:148:9

I take it this means the project requires the creation of an error page beyond what examples/comprehensive/tiny provides.

@phaleth Hope this is gonna be merged. I'm learning from the changes proposed. Thank you.

Awesome! I'm hoping I can contribute to this project going forward. I haven't had the time myself, but now I do (for a while).

arctic-hen7 commented 2 years ago

The tiny example is missing defined error pages, yes. It can work with the defaults in development, but they're disabled in production. I'll add some to it this evening!

allen-woods commented 2 years ago

The tiny example is missing defined error pages, yes. It can work with the defaults in development, but they're disabled in production. I'll add some to it this evening!

👍🏻 Thanks!

Edit: The problems compiling older versions can be fixed using cargo update -p and cargo update --precise.

arctic-hen7 commented 2 years ago

I think the issue might be that we need to get a specific tag of the simple example from the size optimizations plugin's repo. Right now, the Dockerfile seems to always be fetching from main, which could be leading to asymmetries in the engine structure. Pinning to a tag should fix this if I'm right.

arctic-hen7 commented 2 years ago

The error pages are added to the tiny example now, so deployment of that should work now. Please let me know if it doesn't!

phaleth commented 2 years ago

@arctic-hen7 pinning to a tag is already in place. Currently, the codeload link grabs the v0.1.7 tag of the arctic-hen7/perseus-size-opt repo. I think the problem is that versions used in these Dockerfile examples are outdated, but even if all versions are updated to latest greatest bugs might not dissapear and new ones might come up.

https://github.com/arctic-hen7/perseus/blob/f3e3f824530d7103fe776282367e0b872ac0f921/docs/0.3.0-0.3.3/en-US/deploying/docker.md?plain=1#L44-L45

I'm with @allen-woods on the thought the specific perseus version should be able to tell which version of the perseus-size-opt plugin is required, but I have absolutely no clue how to achieve anything like that and actually, I still think that the perseus-size-opt plugin is just an extra effort. I remember once frequent updates to perseus started comming in I always rather opted out of using the perseus-size-opt plugin and used wee_alloc directly. Best of luck with all the fiddling.

arctic-hen7 commented 2 years ago

So it is, I misread that codeload line I think. @allen-woods note that Bonnie is just a command runner, it won't be crashing, it'll be the underlying commands it's running.

I think it might be best to leave the size optimizations plugin completely out of it, since I'm very likely to remove it altogether in the next version, which removes .perseus/, making it a little unnecessary. That's the simplest solution probably.

allen-woods commented 2 years ago

Thank you @phaleth and @arctic-hen7 for both of your input during this process.

I believe I have isolated the problem that's been common across all of the failed builds.

After running cargo install perseus-cli --version <version>, the dependencies need to be locked by running cargo update with both the -p and --precise options. I figured this out when attempting to compile perseus-cli v0.3.1 and it compiled perseus v0.3.5 as a dependency. 😮

I'll get to work on this tonight and try to get v0.3.3 and at least one of the bonnie branch tests working. This should be the "one fix to fix them all".

allen-woods commented 2 years ago

I am proud to announce that I have completed my work on all of the dockerfile docs.

I have personally tested this documentation and confirm that everything 'Just Works™', with the following minor caveat:

Currently, perseus-size-opt lacks support for perseus v0.4.0-beta.1 syntax, causing examples/simple in that repo to fail. Once this issue closes, none of the deployments will fail.

There are two bugs I provided patches for in the Dockerfile examples I wrote.

Edit: I have tested these builds on a first generation M1 MacBook Pro, so there are no issues on M1 machines.

arctic-hen7 commented 2 years ago

Regarding the plugin support in v0.4.x, please see here. Could you show me where in bonnie.toml the unescaped backticks were?

In terms of problem 2, that's due to a longstanding wasm-bindgen issue that means useless warnings get generated without that attribute (including the !). I don't think it's causing a problem per se, it'll likely be perseus tinker that's causing the problem by inserting some stuff into .perseus/lib.rs above that attribute, rendering its position invalid. I seem to recall encountering this issue in the past and being able to deal with it very easily like so in the plugin itself:

let lib_contents_with_wee_alloc = lib_contents.replace(
        "#![allow(clippy::unused_unit)]",
        &format!("#![allow(clippy::unused_unit)]\n{}\n", WEE_ALLOC_DEF),
);

That code is still there, so I'm not sure why that would break. Also, the website is running on v0.3.5 with the plugin without problems, which means there might be something deeper going on here. Could you post the contents of .perseus/lib.rs after running perseus tinker inside the container? That should show if the plugin is misbehaving...

allen-woods commented 2 years ago

Regarding the plugin support in v0.4.x, please see here.

Thank you, I had no idea that was in the roadmap.

Could you show me where in bonnie.toml the unescaped backticks were?

Locations of unescaped backticks are as follows:

bonne.toml

In terms of problem 2, that's due to a longstanding wasm-bindgen issue that means useless warnings get generated without that attribute (including the !). I don't think it's causing a problem per se, it'll likely be perseus tinker that's causing the problem by inserting some stuff into .perseus/lib.rs above that attribute, rendering its position invalid. I seem to recall encountering this issue in the past and being able to deal with it very easily like so in the plugin itself:

let lib_contents_with_wee_alloc = lib_contents.replace(
        "#![allow(clippy::unused_unit)]",
        &format!("#![allow(clippy::unused_unit)]\n{}\n", WEE_ALLOC_DEF),
);

That code is still there, so I'm not sure why that would break. Also, the website is running on v0.3.5 with the plugin without problems, which means there might be something deeper going on here. Could you post the contents of .perseus/lib.rs after running perseus tinker inside the container? That should show if the plugin is misbehaving...

Please advise if the following data indicates the wasm-bindgen bug is related to, or will be solved by the deprecation of perseus-size-opt.

The error mesage as printed by docker buildx build ...:

#24 105.2    Compiling perseus-size-opt-example-simple v0.1.7 (/app/simple)
#24 105.2    Compiling perseus-engine v0.3.5 (/app/simple/.perseus)
#24 105.2 error: an inner attribute is not permitted in this context
#24 105.2  --> src/lib.rs:3:1
#24 105.2   |
#24 105.2 3 | #![allow(clippy::unused_unit)] // rustwasm/wasm-bindgen#2774 awaiting next `wasm-bindgen` release
#24 105.2   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#24 105.2 ...
#24 105.2 6 | pub use app::__perseus_main as main;
#24 105.2   | ------------------------------------ the inner attribute doesn't annotate this `use` import
#24 105.2   |
#24 105.2   = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files
#24 105.2 help: to annotate the `use` import, change the attribute from inner to outer style
#24 105.2   |
#24 105.2 3 - #![allow(clippy::unused_unit)] // rustwasm/wasm-bindgen#2774 awaiting next `wasm-bindgen` release
#24 105.2 3 + #[allow(clippy::unused_unit)] // rustwasm/wasm-bindgen#2774 awaiting next `wasm-bindgen` release
#24 105.2   |
#24 105.2
#24 105.2 error: could not compile `perseus-engine` due to previous error
#24 105.2 Error: Compiling your crate to WebAssembly failed
#24 105.2 Caused by: failed to execute `cargo build`: exited with exit status: 101

Unabridged contents of ./.perseus/src/lib.rs within the container:

# pwd
/app/simple
# cat -n ./.perseus/src/lib.rs
     1  #[global_allocator]
     2  static ALLOC: wee_alloc::WeeAlloc = wee_alloc::WeeAlloc::INIT;
     3  #![allow(clippy::unused_unit)] // rustwasm/wasm-bindgen#2774 awaiting next `wasm-bindgen` release
     4
     5  // The user should use the `main` macro to create this wrapper
     6  pub use app::__perseus_main as main;
     7
     8  use perseus::{
     9      checkpoint, create_app_route,
    10      internal::{
    11          router::{PerseusRouter, PerseusRouterProps},
    12          shell::get_render_cfg,
    13      },
    14      plugins::PluginAction,
    15      templates::TemplateNodeType,
    16  };
    17  use sycamore::prelude::view;
    18  use wasm_bindgen::{prelude::wasm_bindgen, JsValue};
    19
    20  /// The entrypoint into the app itself. This will be compiled to Wasm and actually executed, rendering the rest of the app.
    21  #[wasm_bindgen]
    22  pub fn run() -> Result<(), JsValue> {
    23      let app = main();
    24      let plugins = app.get_plugins();
    25
    26      checkpoint("begin");
    27      // Panics should always go to the console
    28      std::panic::set_hook(Box::new(console_error_panic_hook::hook));
    29
    30      plugins
    31          .functional_actions
    32          .client_actions
    33          .start
    34          .run((), plugins.get_plugin_data());
    35      checkpoint("initial_plugins_complete");
    36
    37      // Get the root we'll be injecting the router into
    38      let root = web_sys::window()
    39          .unwrap()
    40          .document()
    41          .unwrap()
    42          .query_selector(&format!("#{}", app.get_root()))
    43          .unwrap()
    44          .unwrap();
    45
    46      // Create the route type we'll use for this app, based on the user's app definition
    47      create_app_route! {
    48          name => AppRoute,
    49          // The render configuration is injected verbatim into the HTML shell, so it certainly should be present
    50          render_cfg => &get_render_cfg().expect("render configuration invalid or not injected"),
    51          // TODO avoid unnecessary allocation here (major problem!)
    52          // The `G` parameter is ambient here for `RouteVerdict`
    53          templates => &main::<G>().get_templates_map(),
    54          locales => &main::<G>().get_locales()
    55      }
    56      // Create a new version of the router with that
    57      type PerseusRouterWithAppRoute<G> = PerseusRouter<G, AppRoute<TemplateNodeType>>;
    58
    59      // Set up the properties we'll pass to the router
    60      let router_props = PerseusRouterProps {
    61          locales: app.get_locales(),
    62          error_pages: app.get_error_pages(),
    63      };
    64
    65      sycamore::render_to(
    66          move || {
    67              view! {
    68                  // Actually render the router
    69                  // The Perseus router includes our entire app, and is, for all intents and purposes, the app itself
    70                  PerseusRouterWithAppRoute(router_props)
    71              }
    72          },
    73          &root,
    74      );
    75
    76      Ok(())
    77  }
arctic-hen7 commented 2 years ago

Yes, I think that will all be solved by the deprecation of the size opts plugin. Usefully, those optimizations are now bundled into perseus deploy by default! (Or they will be in the next beta.)

As for those backtick issues, I'll take a look at those soon. Thanks!

allen-woods commented 2 years ago

Yes, I think that will all be solved by the deprecation of the size opts plugin. Usefully, those optimizations are now bundled into perseus deploy by default! (Or they will be in the next beta.)

Glad to hear that.

As for those backtick issues, I'll take a look at those soon. Thanks!

Glad to be of help. 😸

I have identified a bug when building docker images using the option --platform linux/amd64. It causes the typenum crate to fail to compile due to an invalid memory reference. Not sure why, so I avoid passing that option.

As of my latest commit I have made changes to the perseus-size-opt example in the 0.3.4 docs as follows:

Currently, the perseus-size-opt example for 0.3.4 is very nearly as far as I can optimize. Adding support for command-line arguments via ARG might be a nice addition, pending your feedback of course.

What may help make the deployment on Docker more user friendly is the creation of official images.

arctic-hen7 commented 2 years ago

Fantastic! Yeah, I think official images would be very helpful. Let me know when you think these are done, and I will look into publishing them to the official Docker registry!

Also, v0.4.0 is pretty stable on optimizations now I think, so if you want to do that Dockerfile as well, you're more thwn welcome to! (No pressure of you don't, of course.) The only major differences are that any optimizations go in the project root Cargo.toml, since the .perseus/ folder is gone, and size optimizations are built into the CLI. That does mean wee_alloc needs to be set up manually though. Docs for optimizations should be about done in the next section of the docs on the website for all this.

allen-woods commented 2 years ago

Per commit 7c87926, I have made the following changes in 0.3.4 docs:

I made this last change because, in practice, running the following commands are synonymous:

More importantly, however, this change proved necessary to unify version numbers for dependencies across the entire framework. These dependencies are as follows:

In certain situations, and depending on the example, deployments can fail because of incompatible versions of these dependencies. Installing binaries with cargo unfortunately involves incompatibilities that are "baked in" at the registry! 😢

This way, uniformity of semvers can be achieved, as well as allow for greater flexibility when making adjustments to an example. That said, I only anticipate needing to provide this approach for versions 0.3.5 and lower with respect to perseus-size-opt implementations specifically.

I look forward to having all versions of the docker deployment documentation completed in the very near future. I'm revising the deployed examples to better represent the framework's capabilities, so work is progressing a touch slower than expected.

arctic-hen7 commented 2 years ago

@allen-woods, it's been a while, are you still working on this? I'm happy to accept a pared back version if that's more convenient for you.

erlend-sh commented 2 years ago

Worth noting that Docker WASM is now a thing: https://docs.docker.com/desktop/wasm/

Perhaps a sensible next step of exploration after this PR is merged?

arctic-hen7 commented 2 years ago

@erlend-sh to my understanding, Perseus' usage of Docker right now is purely for engine-side work, where Wasm isn't really relevant right now.

That having been said, especially given the growing prevalence of server-side Wasm, I think this is an excellent idea. (It would also allow Perseus to integrate with Lunatic, a new async framework.) This would require changing every target gate in Perseus, however, to make sure the compiler understands the difference between browser-side Wasm and engine-side Wasm. A simple find and replace should achieve this though.

arctic-hen7 commented 1 year ago

@allen-woods are you likely to continue work on this any time soon, or shall I close this?

arctic-hen7 commented 1 year ago

@allen-woods I'm going to close this, it's been quite a while, but let me know if you (or anybody else!) want to pick this back up, and I'll happily reopen it.