Skip to content

Make a new standard template variable "CYLC_SOURCE_DIRECTORY"#6944

Merged
oliver-sanders merged 8 commits intocylc:masterfrom
wxtim:feat.WORKFLOW_SRC_DIR-template-var
Sep 9, 2025
Merged

Make a new standard template variable "CYLC_SOURCE_DIRECTORY"#6944
oliver-sanders merged 8 commits intocylc:masterfrom
wxtim:feat.WORKFLOW_SRC_DIR-template-var

Conversation

@wxtim
Copy link
Member

@wxtim wxtim commented Aug 27, 2025

available to users.

Closes #6830

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 Added CYLC_SOURCE_DIRECTORY template variable to jinja2 doc cylc-doc#868.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim requested a review from oliver-sanders August 27, 2025 14:01
@wxtim wxtim self-assigned this Aug 27, 2025
@wxtim wxtim added this to the 8.6.0 milestone Aug 27, 2025
@wxtim wxtim force-pushed the feat.WORKFLOW_SRC_DIR-template-var branch 2 times, most recently from a29e8ef to fb36184 Compare August 27, 2025 14:03
@wxtim wxtim force-pushed the feat.WORKFLOW_SRC_DIR-template-var branch 2 times, most recently from 33b7c58 to 8418026 Compare August 27, 2025 14:34
@wxtim wxtim requested a review from oliver-sanders September 1, 2025 08:59
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

(I'm sure you were about to do the var name changes, sorry!)

For completeness, should this be added as a job environment variable too?

(Probably NO - we don't want jobs to mess with source dirs, as a matter of principle - but other template vars are also job vars(?), and maybe a valid use case could be conjured up...)

@wxtim
Copy link
Member Author

wxtim commented Sep 2, 2025

For completeness, should this be added as a job environment variable too?

For

  • Other template vars are also job vars(?) - not true - CYLC_VERSION is provided, but not CYLC_TEMPLATE_VARS.

Against

  • We don't want jobs to mess with source dirs, as a matter of principle - Somewhat defeats the point of having
  • maybe a valid use case could be conjured up... - We haven't got a use case and can't come up with one readily - if we add it and someone finds a use case this is the sort of thing that creates maintainance headaches.

So I'm going to say no.

@wxtim wxtim requested a review from hjoliver September 2, 2025 08:40
@wxtim wxtim force-pushed the feat.WORKFLOW_SRC_DIR-template-var branch from 2a6bb49 to 2d4b27d Compare September 2, 2025 08:58
available to users.

Response to review
* Make new variable CYLC_WORKFLOW_SRC_DIR.
* Remove cylc-rose testing.
@wxtim wxtim force-pushed the feat.WORKFLOW_SRC_DIR-template-var branch from 2d4b27d to 43243fb Compare September 2, 2025 09:02
@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 2, 2025

For completeness, should this be added as a job environment variable too?

Nooooo!

We definitely shouldn't be encouraging workflow writers to make use of resources in the workflow src dir.

Note, we already decided this which is why we removed the CYLC_SUITE_DEF_PATH variable!

Exposing the source dir at all is annoying, but I have been convinced that this is necessary for rose-stem type use cases.

@hjoliver
Copy link
Member

hjoliver commented Sep 2, 2025

Nooooo!

🤣 - well that's why I said "probably NO", just checking.

@hjoliver
Copy link
Member

hjoliver commented Sep 2, 2025

@wxtim - you have resolved the first two of my suggestions without applying them. If that was deliberate, I'll argue the point: They're not functional code but should use the actual variable name CYLC_WORKFLOW_SRC_DIR or normal text, e.g. "workflow source directory". Currently they use two different capitalized names: WORKFLOW_SOURCE_DIRECTORY (change log); and CYLC_SOURCE_DIRECTORY (in-code comment).

@wxtim
Copy link
Member Author

wxtim commented Sep 3, 2025

@wxtim - you have resolved the first two of my suggestions without applying them. If that was deliberate, I'll argue the point: They're not functional code but should use the actual variable name CYLC_WORKFLOW_SRC_DIR or normal text, e.g. "workflow source directory". Currently they use two different capitalized names: WORKFLOW_SOURCE_DIRECTORY (change log); and CYLC_SOURCE_DIRECTORY (in-code comment).

Not intentional - Either some sort of regression of failure of mine making the changes locally, or GH having some sort of brain fart and not applying changes I asked it to.

…_DIR.t

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
Comment on lines +39 to +42
cylc vip \
--pause \
--no-run-name \
--workflow-name "${WORKFLOW_NAME}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need to run the workflow for this test?

Suggested change
cylc vip \
--pause \
--no-run-name \
--workflow-name "${WORKFLOW_NAME}"
cylc install \
--no-run-name \
--workflow-name "${WORKFLOW_NAME}"

Copy link
Member Author

Choose a reason for hiding this comment

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

Subsequent test requires the flow-processed.cylc to check that the new variable is available and correct. As far as I can tell, template variables provided by Cylc (as opposed to those stored by rose-suite-cylc-install.conf) are not stored on installation.

wxtim and others added 2 commits September 5, 2025 11:08
…_DIR.t

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@wxtim wxtim requested a review from oliver-sanders September 5, 2025 10:39
Comment on lines +38 to +39
run_ok "${TEST_NAME_BASE}-vip" \
cylc vip \
Copy link
Member

@oliver-sanders oliver-sanders Sep 8, 2025

Choose a reason for hiding this comment

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

Would be good to add a cylc view -p test here (works when I try it manually) to make sure this variable is provided in that context too (note config parsing doesn't happen here so it's a likely breakage).

Would be something like:

run_ok ... cylc view -p
grep_ok ... "${TEST_NAME}.stdout"

@wxtim wxtim requested a review from oliver-sanders September 8, 2025 14:53
…_DIR.t

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

👍

@oliver-sanders oliver-sanders merged commit 4cbe3de into cylc:master Sep 9, 2025
24 of 25 checks passed
@wxtim wxtim deleted the feat.WORKFLOW_SRC_DIR-template-var branch September 23, 2025 09:15
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.

config: provide access to source dir location

3 participants