Fix matching multiline control lines in templates with CRLF line endings

[LordAro]

A missing '\' meant that it would actually allow

% if foo \r
   bar:

in a template file and not match if the file actually had a \r char

[zzzeek]

hey there-

any chance you can provide a test case for this?

[LordAro]

Not easy to do reliably, as it would require specific line endings in the file under test - and git mangles those quite readily with autocrlf (which is my workaround for this in the meantime)

Also, I have no idea how to write mako files, I just came across this while trying to get another project that uses them to build

That said, it should be easy to come up with something - as best as I can tell it affects any multiline "control line" (% ... \) with a \r\n line ending

[zzzeek]

im not really sure I understand the problem.

why would someone write this?

% if foo \r
   bar:

or is the issue that it's a syntax error and it doesnt raise an error? I dont really know what we are trying to achieve here.

[LordAro]

Sorry, I phrased the bug report really unclearly. Without this change, a multiline conditional with Windows CRLF newlines such as:

% if foo and \
     bar:

will never work, as the \r (char 13) means that the multiline statement is not recognised properly, resulting in errors like:

CompileException: Fragment 'if (not property.is_dispatcher and \' is not a partial control statement in file 'C:/builds/dev/libadalang/venv/Lib/site-packages/langkit/templates/properties/def_ada.mako' at line: 315 char: 1

[zzzeek]

So this is why we'd' prefer you open an issue report first w/ a test case. So far I can't reproduce (edit: see below, got it), I've added this test:

diff --git a/test/test_template.py b/test/test_template.py
index f2ca6b4..32bab5f 100644
--- a/test/test_template.py
+++ b/test/test_template.py
@@ -32,6 +32,26 @@ class ctx:
         pass
 
 
+class MiscTest(TemplateTest):
+    def test_crlf_linebreaks(self):
+
+        crlf = """
+<%
+    foo = True
+    bar = True
+%>
+% if foo and \
+     bar:
+     foo and bar
+%endif
+"""
+        crlf = crlf.replace("\n", "\r\n")
+        breakpoint()
+        self._do_test(
+            Template(crlf),
+            "\r\n\r\n     foo and bar\r\n"
+        )
+
 class EncodingTest(TemplateTest):
     def test_escapes_html_tags(self):
         from mako.exceptions import html_error_template

no failure. note the string above that we are running as a template is:

'\r\n<%\r\n    foo = True\r\n    bar = True\r\n%>\r\n% if foo and      bar:\r\n     foo and bar\r\n%endif\r\n'

that's the pattern you're referring towards, right?

so to move forward here we need an example template that reproduces the error, so that a test can be added, otherwise the code we have here could be reverted one day and we'd not know it.

[zzzeek]

oh i did the test wrong, didnt get the backslash in, moment

[zzzeek]

hi -

I forgot to escape the string, now it reproduces. add this to your patch please:

diff --git a/test/test_template.py b/test/test_template.py
index f2ca6b4..2d3d9e4 100644
--- a/test/test_template.py
+++ b/test/test_template.py
@@ -32,6 +32,25 @@ class ctx:
         pass
 
 
+class MiscTest(TemplateTest):
+    def test_crlf_linebreaks(self):
+
+        crlf = r"""
+<%
+    foo = True
+    bar = True
+%>
+% if foo and \
+     bar:
+     foo and bar
+%endif
+"""
+        crlf = crlf.replace("\n", "\r\n")
+        self._do_test(
+            Template(crlf),
+            "\r\n\r\n     foo and bar\r\n"
+        )
+
 class EncodingTest(TemplateTest):
     def test_escapes_html_tags(self):
         from mako.exceptions import html_error_template

[LordAro]

Done.

[sqla-tester]

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision e79ebab of this pull request into gerrit so we can run tests and reviews and stuff

[sqla-tester]

New Gerrit review created for change e79ebab: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3307

[zzzeek]

the upcoming 1.2 release will leave Python 2 forever and is also not set to be released right now until we finish it up. i will try to get this published as 1.1.6 which will require some changelog tinkering.

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3307 has been merged. Congratulations! :)

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3308 has been merged. Congratulations! :)