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

[Regression] Doesn't build without top-level templates directory #2150

Closed
vimpostor opened this issue Mar 23, 2023 · 4 comments
Closed

[Regression] Doesn't build without top-level templates directory #2150

vimpostor opened this issue Mar 23, 2023 · 4 comments
Labels

Comments

@vimpostor
Copy link

vimpostor commented Mar 23, 2023

Since version 0.17.1 zola requires the top-level /templates directory to exist, even if a theme is used. Otherwise it will fail with:

Error: Failed to build the site
Error: Error parsing templates from the /templates directory
Error: Reason: Io error while writing rendered value to output: NotFound
Error: Reason: No such file or directory (os error 2)

This is because now tera handles globs differently, as discussed in Keats/tera#819 (comment).

I believe that this is not very user-friendly, as it can quickly lead to unexpected build failures with the default setup as follows:

  • Run zola init mysite
  • Run zola serve
  • The official default index.html says that you should either add an index.html or add a theme to use
  • You decide for the second option and add a theme, which leaves the top level templates directory empty
  • It builds fine, so you push to a git repo
  • Now pulling on a different machine or on CI will cause zola to fail building the site, because there is no templates directory checked into git

In my opinion this is not optimal, and zola should still work if there is no top-level templates directory.

The following patch fixes the problem from zola's side:

diff --git a/components/templates/src/lib.rs b/components/templates/src/lib.rs
index 6441ff49..e2747bfc 100644
--- a/components/templates/src/lib.rs
+++ b/components/templates/src/lib.rs
@@ -47,8 +47,7 @@ pub fn load_tera(path: &Path, config: &Config) -> Result<Tera> {
 
     // Only parsing as we might be extending templates from themes and that would error
     // as we haven't loaded them yet
-    let mut tera =
-        Tera::parse(&tpl_glob).context("Error parsing templates from the /templates directory")?;
+    let mut tera = Tera::parse(&tpl_glob).unwrap_or(Tera::default());
 
     if let Some(ref theme) = config.theme {
         // Test that the templates folder exist for that theme

However it's not really a good solution, because it will shadow actual parsing problems if /templates exists and there are real problems. This is why I believe this problem is better fixed on tera's side, i.e. making invalid globs just return the empty set instead of an error, which is how it is done in most languages.

@vimpostor vimpostor changed the title [Regression] Doesn't build without top-level directory [Regression] Doesn't build without top-level templates directory Mar 23, 2023
@Keats Keats added the bug label Mar 26, 2023
@iwahbe
Copy link

iwahbe commented Mar 26, 2023

This is extremely unfriendly, especially since you can't check an empty folder into git.

We can do better then just ignoring all errors here. At the very least, we can check the err kind and apply defaults only if the expected file is missing:

 components/templates/src/lib.rs | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/components/templates/src/lib.rs b/components/templates/src/lib.rs
index 6441ff49..7f1aaa53 100644
--- a/components/templates/src/lib.rs
+++ b/components/templates/src/lib.rs
@@ -5,7 +5,7 @@ use std::path::Path;
 
 use config::Config;
 use libs::once_cell::sync::Lazy;
-use libs::tera::{Context, Tera};
+use libs::tera::{self, Context, Tera};
 
 use errors::{bail, Context as ErrorContext, Result};
 use utils::templates::rewrite_theme_paths;
@@ -47,8 +47,15 @@ pub fn load_tera(path: &Path, config: &Config) -> Result<Tera> {
 
     // Only parsing as we might be extending templates from themes and that would error
     // as we haven't loaded them yet
-    let mut tera =
-        Tera::parse(&tpl_glob).context("Error parsing templates from the /templates directory")?;
+    let mut tera = match Tera::parse(&tpl_glob) {
+        Ok(t) => t,
+        Err(e) if matches!(e.kind, tera::ErrorKind::Io(std::io::ErrorKind::NotFound)) => {
+            Tera::default()
+        }
+        Err(e) => {
+            return Err(e).context("Error parsing templates from the /templates directory");
+        }
+    };
 
     if let Some(ref theme) = config.theme {
         // Test that the templates folder exist for that theme

I'm going to open a PR.

iwahbe added a commit to iwahbe/zola that referenced this issue Mar 26, 2023
@iwahbe iwahbe mentioned this issue Mar 26, 2023
3 tasks
iwahbe added a commit to iwahbe/zola that referenced this issue Mar 26, 2023
iwahbe added a commit to iwahbe/zola that referenced this issue Mar 26, 2023
@Keats
Copy link
Collaborator

Keats commented Mar 26, 2023

especially since you can't check an empty folder into git.

You can, you just need to add a dummy file in it (usually an empty .gitkeep by convention). I agree it needs to be fixed though

iwahbe added a commit to iwahbe/zola that referenced this issue Mar 27, 2023
@proofy
Copy link

proofy commented Mar 29, 2023

As a supplement: This error also occurs if a theme is defined. Then the template directory should generally not matter, or?

@Keats
Copy link
Collaborator

Keats commented Mar 30, 2023 via email

ariasanovsky added a commit to ariasanovsky/ariasanovsky.github.io that referenced this issue Mar 31, 2023
vimpostor added a commit to vimpostor/tera that referenced this issue Apr 6, 2023
Regression was introduced in 9479c28.

Since that commit providing an invalid glob to tera will make it fail to
create an instance, first (in that commit) by making it panic and since
1f95878 by making it return an error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails to return the
empty set again when encountering an invalid glob.

Even more so, since this caused a lot of regressions already, including
in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
vimpostor added a commit to vimpostor/tera that referenced this issue Apr 6, 2023
Regression was introduced in 9479c28.

Since that commit, providing an invalid glob to tera will make it fail
to create an instance, first (in that commit) by making it panic and
since 1f95878 by making it return an
error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails to return the
empty set again when encountering an invalid glob.

We should especially restore the previous behaviour, as this caused a
lot of regressions already, including in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
vimpostor added a commit to vimpostor/tera that referenced this issue Apr 6, 2023
Regression was introduced in 9479c28.

Since that commit, providing an invalid glob to tera will make it fail
to create an instance, first (in that commit) by making it panic and
since 1f95878 by making it return an
error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails.

We should especially restore the previous behaviour, as this caused a
lot of regressions already, including in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
vimpostor added a commit to vimpostor/tera that referenced this issue Apr 10, 2023
Regression was introduced in 9479c28.

Since that commit, providing an invalid glob to tera will make it fail
to create an instance, first (in that commit) by making it panic and
since 1f95878 by making it return an
error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails.

We should especially restore the previous behaviour, as this caused a
lot of regressions already, including in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
Sparrow0hawk added a commit to Sparrow0hawk/sparrow0hawk.github.io that referenced this issue May 3, 2023
araruna added a commit to araruna/araruna.github.io that referenced this issue Jul 1, 2023
tonykolomeytsev added a commit to tonykolomeytsev/tonykolomeytsev.github.io that referenced this issue Aug 26, 2023
Congee added a commit to Congee/blog that referenced this issue Nov 1, 2023
Congee added a commit to Congee/blog that referenced this issue Nov 1, 2023
Congee added a commit to Congee/blog that referenced this issue Nov 1, 2023
Congee added a commit to Congee/blog that referenced this issue Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants