Switch to std::path::absolute to avoid symlinking template paths

[kaepr]

In our build system, we use content addressable storage, where the template files lose their extensions. For e.g. for the below

#[derive(Template)]
#[template(path = "sidebar.html")]

Which might become a file such as

path/to/some/location/YGBgcnVzdAojW2Rlcml2ZShUZW1wbGF0ZSldCiNbdGVtcGxhdGUocGF0aCA9ICJzaWRlYmFyLmh0bWwiKV0KUmFuZG9tCmBgYAo=

As we have lost the extension, it defaults to empty strings here. So instead of applying the default HTML filter to escape symbols, it defaults to None.

The fix suggested switches to absolute, which avoids symlinking the template path. This preserves the file extension and the correct escaper is applied.

I am seeing other issues such as #288, #708 which also revolve around absolute / canonical paths. All the tests do pass with this change, but there are other workarounds to preserve the file path, which can possibly avoid affecting their systems.

For e.g. storing the file extension before passing config.find_template in the TemplateInput::new.

        } = args;

        let raw_ext = match &source {
            #[cfg(feature = "external-sources")]
            Source::Path(p) => Path::new(p.as_ref()).extension().and_then(|s| s.to_str()),
            _ => None,
        };

        // ...
        // Match extension against defined output formats

        let escaping = escaping
            .as_deref()
            .or_else(|| path.extension().and_then(|s| s.to_str()))
            .or_else(|| raw_ext) // it might be better to have this check first ?
            .unwrap_or_default();

[GuillaumeGomez]

I think it's the correct fix, although for the error display, would be better to keep the original path. Or eventually the original followed by the absolute path in parens.

[kaepr]

Added changes to reflect path names for the other tests, and added a new test case to test the symlink behavior.

[GuillaumeGomez]

That's not what I meant in my comment.

[kaepr]

Reverted changes to the test files. My bad.

[GuillaumeGomez]

I think it's the correct fix, although for the error display, would be better to keep the original path. Or eventually the original followed by the absolute path in parens.

I should be more precise. This test output will need to be updated, however we need to make it clear to the users why this cycle error occurred. What I had in mind was something like:

cyclic dependency in graph [
    "$DIR/cycle2.html" (absolute path: "$WORKSPACE/target/tests/trybuild/askama_testing/templates/cycle2.html") --> "$DIR/templates/cycle1.html/" (absolute path: "$WORKSPACE/target/tests/trybuild/askama_testing/templates/cycle1.html")
   ...
]

So the original path followed by the absolute path in parens.

[kaepr]

Ah got it now. It might require to keep both original and absolute path values from the find_templates's return type, or make some adjustments in the input.rs file so that the error message constructors have both the paths.

Will share the commit for this soon.

[kaepr]

Have updated the error messages in the latest commit.

[GuillaumeGomez]

Apart from the nits, looks all good!

[GuillaumeGomez]

Ah right, windows gonna be an issue (as usual).

[kaepr]

Figuring that out, I am expecting running things with --target as windows should allow me to replicate it.

Given that we have changed path resolution logic itself, the solution should most likely be updating the tests cases ? Will try things locally and update.

[kaepr]

It seems I can't test things locally as Windows. I only have a Mac. Tried out different ways but didn't work out.

Updated the tests to use absolute instead. This should be the fix.

[GuillaumeGomez]

Different failure this time:

---- symlink_preserves_extension_for_escaping stdout ----

thread 'symlink_preserves_extension_for_escaping' (3008) panicked at testing\tests\paths.rs:30:5:
assertion `left == right` failed
  left: "Hello, <>&"'!\r"
 right: "Hello, <>&"'!"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

But at least your fix for the paths display worked, well done!

[kaepr]

testing/templates/symlink-target text eol=lf should satisfy windows :)

[GuillaumeGomez]

Code looks good, CI is happy. Time to merge. Thanks a lot!

[kaepr]

Thanks a lot for the reviews !