Raise an error if template variables are set with no shebang by wxtim · Pull Request #6948 · cylc/cylc-flow

wxtim · 2025-08-28T13:23:33Z

line in the flow.cylc.

Fixes an otherwise unreported bug.

Real world example of bug:

#!/bin/bash
TMP=$(mktemp -d)
echo Files extracted to ${TMP}
mkdir -p "${TMP}/."

cat > "${TMP}/./flow.cylc" <<__ICI__
[scheduler]
    allow implicit tasks = true

[scheduling]
    initial cycle point = {{ STARTDATE }}T0000Z

    [[graph]]
        P1M = foo

__ICI__

cat > "${TMP}/./rose-suite.conf" <<__ICI__
[template variables]
STARTDATE="20000101"

__ICI__

cylc val ${TMP}
# .... lots of traceback

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
Tests are included (or explain why tests are not needed).
Changelog entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

but no shebang line is set in the flow.cylc.

oliver-sanders

This change is overzealous for this example:

# ./flow.cylc
[scheduling]
  [[graph]]
    R1 = foo

[runtime]
  [[foo]]

# ./rose-suite.conf
[env]
FOO=bar

$ cylc val . -s FOO=1
TemplateVarLanguageClash: No shebang line (#!jinja2) in config file.

But underzealous for this one

# ./flow.cylc
[scheduling]
  [[graph]]
    R1 = foo

[runtime]
  [[foo]]

$ cylc val . -s FOO=1
Valid for cylc-8.5.2.dev

hjoliver · 2025-09-01T00:00:03Z

cylc/flow/parsec/fileparse.py

    TEMPLATE_VARIABLES: {},
    TEMPLATING_DETECTED: None
 }
+JINJA2_SHEBANG = '#!jinja2'


Note we allow #!Jinja2 (capitalized) as well... but this is just for insertion, not detection?

Where it's used for detection it'd be used with ~~JINJA2_SHEBANG = var.lower()~~

Correction: It's a regex

wxtim · 2025-09-01T09:03:24Z

Converted to draft.

wxtim · 2025-09-02T16:05:03Z

I'm slightly worried that this has the potential to throw out a lot of valid workflows, just because they have a rose-suite.conf. Perhaps it should be a warning with a plan to make an error in future?

@oliver-sanders what think you?

oliver-sanders · 2025-09-02T16:36:11Z

@oliver-sanders what think you?

This isn't a biggie, I'm not too concerned.

We shouldn't add a check which has the potential to fail, so if there ain't a good way to do it, let's not do it.

oliver-sanders · 2025-09-05T09:44:47Z

tests/functional/events/11-cycle-task-event-job-logs-retrieve/flow.cylc

@@ -1,3 +1,4 @@
+#!jinja2


Why?

This config doesn't use Jinja2!

But template variables are supplied - looks like there's vestigial garbage in the test.

cylc/flow/parsec/fileparse.py

tests/functional/platforms/05-host-to-platform-upgrade-fail/flow.cylc

tests/functional/remote/01-file-install.t

oliver-sanders · 2025-09-05T09:46:32Z

cylc/flow/parsec/fileparse.py

            flines = jinja2process(
                fpath, flines, fdir, template_vars
            )
+        elif template_vars.keys() - {"CYLC_VERSION", "CYLC_TEMPLATE_VARS"}:


Does cylc-rose provide default template vars even if templating was not configured?

If you turn Cylc Rose on it provides template variables. You turn it on by adding a blank rose-suite.conf.

So adding a rose-suite.conf file will force the user to add a Jinja2 shebang line, even if they are not using Jinja2 in the config?

I don't think there's a simple/sane* way around that. If you think that's a deal breaker IMO we just call this PR off. It's too small an issue for me to spend much more time on it.

* Non simple ways

with supress: from cylc.rose.utilites import STANDARD_VARS (less general change, but I don't like adding a single plugin exception)

Modify the plugin interface (more general, but I don't want to change the interface!)

I don't think there's a simple/sane* way around that.

I think forcing people to add Jinja2 shebangs where they aren't needed is going to be more of a problem than failing to spot when shebangs aren't needed but should have been added.

wxtim · 2025-09-05T13:44:00Z

Solutions worse than problem. Bug recorded at #6971

hjoliver · 2025-09-08T23:12:10Z

I guess this is to be closed then?

wxtim · 2025-09-09T09:01:43Z

I guess this is to be closed then?

Thought I had

wxtim requested review from MetRonnie and oliver-sanders August 28, 2025 13:23

wxtim added this to the 8.5.2 milestone Aug 28, 2025

wxtim force-pushed the fix.template-vars-no-shebang branch from 3b4b88d to 58b6cdb Compare August 28, 2025 13:25

wxtim mentioned this pull request Aug 28, 2025

Ensure that malformed datetime points with > 1 "T" raise the correct error metomi/isodatetime#266

Merged

wxtim force-pushed the fix.template-vars-no-shebang branch 2 times, most recently from c236a30 to 7de3421 Compare August 28, 2025 14:15

Raise an error if template variables are set

85d9252

but no shebang line is set in the flow.cylc.

wxtim force-pushed the fix.template-vars-no-shebang branch from 7de3421 to 85d9252 Compare August 28, 2025 14:18

oliver-sanders requested changes Aug 29, 2025

View reviewed changes

hjoliver reviewed Sep 1, 2025

View reviewed changes

wxtim marked this pull request as draft September 1, 2025 09:02

MetRonnie removed their request for review September 1, 2025 14:45

test changes (may want rollback if we go for warning not error)

39bb3cd

MetRonnie modified the milestones: 8.5.2, 8.5.x Sep 3, 2025

wxtim added 2 commits September 3, 2025 11:30

fix functional tests

c2fc643

fix functional tests

b2481ce

wxtim requested review from hjoliver and oliver-sanders September 3, 2025 11:44

wxtim marked this pull request as ready for review September 3, 2025 11:44

oliver-sanders assigned wxtim Sep 5, 2025

oliver-sanders reviewed Sep 5, 2025

View reviewed changes

wxtim added 2 commits September 5, 2025 11:43

remove comments

1154b0a

remove vestigial variable passing to test workflows

95b5efb

wxtim mentioned this pull request Sep 5, 2025

Template variables set but shebang line absent #6971

Open

wxtim closed this Sep 9, 2025

wxtim deleted the fix.template-vars-no-shebang branch September 9, 2025 09:02

oliver-sanders removed this from the 8.5.x milestone Sep 9, 2025

Conversation

wxtim commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Real world example of bug:

Uh oh!

oliver-sanders left a comment

Choose a reason for hiding this comment

Uh oh!

hjoliver Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxtim Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxtim commented Sep 1, 2025

Uh oh!

wxtim commented Sep 2, 2025

Uh oh!

oliver-sanders commented Sep 2, 2025

Uh oh!

oliver-sanders Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

wxtim Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliver-sanders Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

wxtim Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

oliver-sanders Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

wxtim Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliver-sanders Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

wxtim commented Sep 5, 2025

Uh oh!

hjoliver commented Sep 8, 2025

Uh oh!

wxtim commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wxtim commented Aug 28, 2025 •

edited

Loading

hjoliver Sep 1, 2025 •

edited

Loading

wxtim Sep 1, 2025 •

edited

Loading

wxtim Sep 5, 2025 •

edited

Loading