Skip to content

Conversation

@fasterthanlime
Copy link
Contributor

Most likely we need to contribute back to minijinja2 to get proper tracking information for "undefined errors" because right now all we see is this:

CleanShot 2024-10-29 at 14 02 49@2x

@fasterthanlime fasterthanlime force-pushed the strict-undefined-behavior branch 9 times, most recently from 18a45b1 to fe96de9 Compare November 1, 2024 15:04
@fasterthanlime
Copy link
Contributor Author

Current state of this PR: the corresponding minjinja fix has landed in their main branch, but not in a point release yet, which means our dep now looks like this:

CleanShot 2024-11-01 at 16 04 50@2x

So I don't love that for us and this PR can probably wait for a new minjinja to land (releases seem regular)


BUT! it looks awesome and is very useful: I pulled in color-backtrace for coloring and filtering and this helps me at least (I'm curious to see what @mistydemeo and @duckinator think about it specifically) narrow down what's wrong almost immediately:

CleanShot 2024-11-01 at 16 08 54@2x

color-backtrace is awesome (and the big dependency here is backtrace, which we already have) and has a RUST_LIB_BACKTRACE=full mode that even shows you the Rust source code:

CleanShot 2024-11-01 at 16 09 48@2x

Even though I'd like to wait for a minjinja point release to drop, I'm gonna stack my other PRs on top of this one because, damn, that's good tooling.

@fasterthanlime
Copy link
Contributor Author

Note 1: I've forced backtrace capture, even if you haven't otherwise exported RUST_LIB_BACKTRACE=1 because I consider this equivalent to a panic, it might happen in CI, we're telling our users to report it and when they do, we'll want as much info as possible.


Note 2: this PR also fixes the few instances of us violating "strict undefined behavior", minjinja is going to get a little legalistic on us — the only thing y'all need to know is that if it complains something like:

      undefined value (in installer/npm/run.js:4)
   ╭─[4:1]
 3 │ const { run } = require("./binary");
 4 │ {%- if bin %}
   · ─────────────
 5 │ run({{ bin }});
   ╰────

Then you need if bin is defined, and the reason this happens is usually because the field is defined something like this:

struct Blah {
    #[serde(skip_serializing_if = "Option::is_none")]
    pub bin: Option<String>,
}

But in the actual case of bin? it's because it's rendered without any "context/globals" object once, as evidenced later in the code:

// There's a run.js.j2 that we actually want to render once-per-binary, so remove this copy
let run_js_path = Utf8Path::new(RUN_JS);
files
.remove(run_js_path)
.expect("npm template didn't have a run.js!?");

ie. while "rendering the directory template", it gets rendered once with zero context, and then it's removed and re-rendered with the correct context. That's a hack and I hate it, but with just if bin is defined instead of if bin, we can go ahead anyway:

{%- if bin is defined %}
run({{ bin }});
{%- endif %}

@fasterthanlime
Copy link
Contributor Author

After discussing in Discord, we'll publish minjinja as axominijinja and switch to that for the next release, then switch back when it's done.

@fasterthanlime fasterthanlime changed the title Explore 'strict undefined behavior' (for minjinja2) Enable jinja "strict undefined behavior", fix templates, improve reporting Nov 1, 2024
@fasterthanlime
Copy link
Contributor Author

Taking this out of draft since it's ready for being merged imho!

@fasterthanlime fasterthanlime marked this pull request as ready for review November 1, 2024 15:44
@fasterthanlime fasterthanlime force-pushed the strict-undefined-behavior branch from 0c6a265 to eb4e8ee Compare November 1, 2024 15:48
@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Nov 1, 2024

CleanShot 2024-11-01 at 17 16 27@2x

I caught another template issue (a benign one) while working on #1509 — which mostly tells me the homebrew stuff isn't all covered by unit (or integration) tests

@ashleygwilliams ashleygwilliams added this to the 0.26.0 milestone Nov 1, 2024
@fasterthanlime fasterthanlime force-pushed the strict-undefined-behavior branch 2 times, most recently from 9c9433c to 7b80e2b Compare November 4, 2024 20:50
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

Looking good, but a few comments - one error, and a few other suggestions and questions.

@fasterthanlime fasterthanlime force-pushed the strict-undefined-behavior branch 3 times, most recently from 1abdc8b to d5017d6 Compare November 4, 2024 23:22
---
source: cargo-dist/tests/gallery/dist/snapshot.rs
expression: self.payload
snapshot_kind: text
Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably due to the minijinja update? idk

Copy link
Contributor

Choose a reason for hiding this comment

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

What version of cargo-insta do you have? This was added when Ellen updated a test, which makes me think they might have a newer version than you or I do.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK yeah, this field landed in cargo-insta 1.41 mitsuhiko/insta@0b81773

Prior to that insta only had a single snapshot format so there was no need to specify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll upgrade locally!

@fasterthanlime
Copy link
Contributor Author

@mistydemeo I addressed whitespace issues — in fact, I made sure the template produces exactly the same input as before, just using those nice macros and the new "template input" structs.

I've also added a comment about the jinjaness of the macro (and a link to the surprising-to-find docs), and fixed the endif/end mishap.

Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

Looking good, just a few comments now!

@fasterthanlime
Copy link
Contributor Author

The mac tests are failing because it's trying to install a linuxbrew-only package..

..and make it really friendly to debug those errors.

Also, due to homebrew template changes, dist now collects homebrew dependencies
per-target, rather than globally, which means that when generating installers,
we're no longer mixing up macbrew and linuxbrew dependencies, for example.
@fasterthanlime fasterthanlime force-pushed the strict-undefined-behavior branch from 6b54bd3 to 4c88c10 Compare November 5, 2024 13:39
@fasterthanlime fasterthanlime merged commit 576e266 into main Nov 5, 2024
18 checks passed
@fasterthanlime fasterthanlime deleted the strict-undefined-behavior branch November 5, 2024 14:17
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.

4 participants