migrate to rocket 0.5.0-rc.3 #1795

senekor · 2023-04-07T15:16:57Z

No description provided.

Manishearth · 2023-04-07T15:54:57Z

Dockerfile needs fixing for rust-toolchain it seems

senekor · 2023-04-07T15:19:02Z

Dockerfile

@@ -51,7 +51,7 @@ COPY static /app/static/
 COPY src/styles /app/src/styles/

 ENV ROCKET_PORT 80
-ENV ROCKET_ENV prod
+ENV ROCKET_PROFILE release


From the changelog:

ROCKET_ENV is now ROCKET_PROFILE. A warning is emitted a launch time if the former is set.

senekor · 2023-04-07T15:22:19Z

Rocket.toml

@@ -1,3 +1,3 @@
-[development]
+[debug]


From the changelog:

The default profile for debug builds is now debug, not dev.

senekor · 2023-04-07T15:25:54Z

src/cache.rs

+        task::spawn_blocking(move || {
+            update_cache::<T>(generator)
+                // stringify the error to make it Send
+                .map_err(|e| e.to_string())
+        })
+        .await
+        .map_err(Box::new)?
+        // put the previously stringified error back in a box
+        .map_err(|e| e.as_str().into())


I tried to make the caching more idiomatic for async, but that got very hairy very quickly... after many wasted hours, I gave up. Blocking in the foreground on a request should still only happen once for each cache entry when it's empty the first time.

senekor · 2023-04-07T15:29:54Z

src/caching.rs

+impl CachedNamedFile {
+    pub fn max_age(file: NamedFile, max_age: u32) -> Self {
+        Self {
+            file,
+            header: Header {
+                name: Uncased {
+                    string: "CacheControl".into(),
+                },
+                value: format!("max-age={max_age}").into(),


The CacheDirective was removed from Rocket. (A breaking change not found in the changelog 😞 ) Since this module isn't really used as generically as it was designed, I implemented a simpler, more manual version which is sufficient for how it's used.

I haven't tested it though, I might need to make sure this header actually works as intended.

senekor · 2023-04-07T15:32:17Z

src/category.rs

 use rocket::request::FromParam;

 fn is_category(name: &str) -> bool {
    let mut path = PathBuf::from("templates");
    path.push(name);
-    path.push("index.hbs");
+    path.push("index.html.hbs");


rocket_dyn_templates (previously part of rocket_contrib) sets content type based on the second file extension. So I renamed all the templates, otherwise the html would be sent as text/plain.

senekor · 2023-04-07T15:36:48Z

src/i18n.rs

        let team = rcx
-            .evaluate_in_block_context(name)?
-            .ok_or_else(|| RenderError::new(format!("Cannot find team {}", name)))?;
+            .evaluate(context, name)
+            .map_err(|e| RenderError::from_error(&format!("Cannot find team {}", name), e))?;


This part is basically guesswork on my part. I have zero experience with handlebars, so these helper idioms are unknown to me. evaluate_in_block_context doesn't exist anymore. I found no hints about this in the changelog of handlebars and evaluate isn't even documented in the current version 😞 .

I'm guessing these teams are related to the governance pages though, and they seem to work fine (after some debugging).

senekor · 2023-04-07T15:49:30Z

src/main.rs

    render_category(category, ENGLISH.into())
 }

-#[get("/<locale>/<category>", rank = 11)]
+#[get("/<locale>/<category>", rank = 9)]


Route ranking has received breaking changes. I had to fiddle with a few of the rankings to get some pages to render (category_locale and governance_locale I think).

Maybe I'm biased because dealing with these breaking changes was my first experience dealing with the routes of this site, but relying on framework magic for routing seems like a really bad idea. This site doesn't have an egregious amount of different routes, it seems so much saner to me to just declare the routes explicitly.

I seems like Rocket's system was even abused for optional path segments, namely the locale. Something like that clearly belongs in the query params, no?

Maintaining this URL structure into the future is probably going to be really annoying, especially for static site generation.

senekor · 2023-04-07T15:52:19Z

src/main.rs

+                category_en,
                governance,
                team,
-                production,
+                get_production,


Rocket's macros for the handler definitions seem to work differently now, they introduce modules into the scope that produce naming conflicts...

senekor · 2023-04-07T15:53:21Z

src/redirect.rs

-pub(crate) fn maybe_redirect(segments: Segments<'_>) -> Option<Redirect> {
-    let path = segments.into_path_buf(false).ok()?.to_str()?.to_string();
+pub(crate) fn maybe_redirect(path: Path) -> Option<Redirect> {
+    let path = path.segments().collect::<Vec<_>>().join("/");


I don't think I've tested this .join("/") yet.

senekor · 2023-04-07T15:55:50Z

templates/404.html.hbs

@@ -14,4 +14,4 @@
 </header>

 {{/inline}}
-{{~> (parent)~}}
+{{~> (lookup this "parent")~}}


From the handlebars changelog:

[Fixed] Breaking zero-arity subexpressions support [#433] Zero-arity subexpression no longer resolved as variable. The behaviour is now aligned with handlebarsjs. For instance, {{(parent)}} can no longer access parent field of the context object, use {{lookup this "parent"}} instead. This change applies to partial inclusion, too.

Manishearth

I don't see any issues with this.

Should get autodeployed to https://www-staging.rust-lang.org/ around 10 minutes after merging, we should play with that before we deploy properly

senekor · 2023-04-08T06:38:33Z

@Manishearth oops... can you check the logs?

Manishearth · 2023-04-08T23:04:58Z

@pietroalbini I no longer have access to the website/staging heroku, can you check?

pietroalbini · 2023-04-10T08:31:41Z

2023-04-10T06:13:15.709150+00:00 app[web.1]: Rocket has launched from http://127.0.0.1:28788
2023-04-10T06:14:14.399538+00:00 heroku[web.1]: Error R10 (Boot timeout) -> Web process failed to bind to $PORT within 60 seconds of launch
2023-04-10T06:14:14.427200+00:00 heroku[web.1]: Stopping process with SIGKILL
2023-04-10T06:14:14.601628+00:00 heroku[web.1]: Process exited with status 137
2023-04-10T06:14:14.632825+00:00 heroku[web.1]: State changed from starting to crashed

pietroalbini · 2023-04-10T08:33:43Z

@Manishearth granted back access to Heroku!

senekor · 2023-04-10T08:59:31Z

Hmm. As far as I can tell, the handling of the ROCKET_PORT environment variable did not change. Nothing about it in the changelog, upgrade guide or configuration docs.

The dockerfile specifies

ENV ROCKET_PORT 80

but Heroku's Procfile overrides that...? (I have no experience with Heroku)

web: ROCKET_PORT=$PORT ROCKET_PROFILE=release ./target/release/www-rust-lang-org

Not sure which is correct. Either way, I'm confused as to how that was impacted by the rocket migration.

Manishearth · 2023-04-10T16:53:31Z

I don't think Heroku looks at the dockerfile at all, yeah, that's just there for local dev and CI. I think?

Manishearth · 2023-04-10T17:00:06Z

I'm trying 9eeae68, seems like heroku wants it to bind to 0.0.0.0

Manishearth · 2023-04-10T17:49:50Z

http://www-staging.rust-lang.org/ seems like that worked!

Manishearth · 2023-04-10T17:50:07Z

We should update the Rocket.toml with that instead of using an env var but I'll do that later

Manishearth · 2023-04-10T17:50:55Z

Also we should definitely click around the website and ensure basically all of it works before deploying to prod.

Manishearth · 2023-04-10T17:51:53Z

One thing from the logs:

2023-04-10T17:50:22.913479+00:00 app[web.1]:    >> Parameter guard `category: Category` is forwarding: "No category called <zh-TW>".

2023-04-10T17:50:23.046223+00:00 heroku[router]: at=info method=GET path="/static/styles/vendor_10880690442070639967.css" host=www-staging.rust-lang.org request_id=d7ad0c83-cb43-414f-81f6-33e4bafd572f fwd="24.7.113.20" dyno=web.1 connect=0ms service=1ms status=200 bytes=73930 protocol=http

This might be an expected warning since we have a cascade of forwards.

senekor · 2023-04-10T17:57:53Z

One thing from the logs:

2023-04-10T17:50:22.913479+00:00 app[web.1]:    >> Parameter guard `category: Category` is forwarding: "No category called <zh-TW>".

2023-04-10T17:50:23.046223+00:00 heroku[router]: at=info method=GET path="/static/styles/vendor_10880690442070639967.css" host=www-staging.rust-lang.org request_id=d7ad0c83-cb43-414f-81f6-33e4bafd572f fwd="24.7.113.20" dyno=web.1 connect=0ms service=1ms status=200 bytes=73930 protocol=http

This might be an expected warning since we have a cascade of forwards.

I think so, yes. Clicking through the website with the zh-TW locale, I can't find any route that's broken.

senekor · 2023-04-10T18:15:11Z

I clicked around, making sure I'm hitting every route at least once. Everything seems fine, so the route ranking should be working at least.

Manishearth · 2023-04-11T01:09:05Z

Hit the deploy button, will take a while for Heroku to actually pick it up and deploy it. I'll keep an eye out in the meantime.

Thanks for working on this!

migrate to rocket 0.5.0-rc.3

f13ef48

senekor requested a review from a team as a code owner April 7, 2023 15:16

senekor commented Apr 7, 2023

View reviewed changes

fix dockerfile

9ed278d

Manishearth approved these changes Apr 7, 2023

View reviewed changes

Manishearth merged commit edfc5a9 into rust-lang:master Apr 7, 2023

senekor deleted the rocket-0.5 branch April 8, 2023 06:38

Manishearth mentioned this pull request Apr 11, 2023

Deploy #1802

Merged

		@@ -1,3 +1,3 @@
		[development]
		[debug]

migrate to rocket 0.5.0-rc.3 #1795

migrate to rocket 0.5.0-rc.3 #1795

Uh oh!

Conversation

senekor commented Apr 7, 2023

Uh oh!

Manishearth commented Apr 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

senekor commented Apr 8, 2023

Uh oh!

Manishearth commented Apr 8, 2023

Uh oh!

pietroalbini commented Apr 10, 2023

Uh oh!

pietroalbini commented Apr 10, 2023

Uh oh!

senekor commented Apr 10, 2023

Uh oh!

Manishearth commented Apr 10, 2023

Uh oh!

Manishearth commented Apr 10, 2023

Uh oh!

Manishearth commented Apr 10, 2023

Uh oh!

Manishearth commented Apr 10, 2023

Uh oh!

Manishearth commented Apr 10, 2023

Uh oh!

Manishearth commented Apr 10, 2023

Uh oh!

senekor commented Apr 10, 2023

Uh oh!

senekor commented Apr 10, 2023

Uh oh!

Manishearth commented Apr 11, 2023

Uh oh!

Uh oh!