fix: use destination path as working directory while rendering#2754
fix: use destination path as working directory while rendering#2754nikzart wants to merge 2 commits into
Conversation
While rendering a subproject, the working directory was the directory Copier was invoked from, so filesystem-related Jinja filters and extensions (e.g. `fileglob`) operated relative to it. Change the working directory to the destination path while rendering, creating it eagerly (except in pretend mode) so this also applies when copying into a not-yet-existing folder. External data is now loaded from the absolute destination path, because it may be loaded lazily while rendering, when the working directory is the destination path and a relative `dst_path` would no longer resolve correctly. Fixes copier-org#1708 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| cwd_before = Path.cwd() | ||
| run_update(dst, defaults=True, overwrite=True) | ||
| assert Path.cwd() == cwd_before | ||
| assert (dst / "fileglob.txt").read_text() == "['a.txt', 'version.txt']" |
There was a problem hiding this comment.
I wonder whether a.txt should be listed by the fileglob filter during an update, especially considering #2376 which only replays a fresh copy based on the old template, renders a fresh copy based on the new template, and merges the latter into HEAD given the former, which is the common ancestor (i.e., a "simple" 3-way merge). The new update algorithm implementation in #2376 never copies into the user-specified destination unlike the current update algorithm implementation – and I think that's the only reason why a.txt is listed by fileglob here. When we switch to a final version of #2376, the behavior asserted by this PR might change.
Perhaps the more important question is even: Should the CWD be the user-specified destination during an update, or should it be the destination where files are being rendered? The latter seems more consistent and predictable and less susceptible to unexpected side-effects to me at first glance. Also, updating currently relies on replaying a fresh copy based on the old template, but from my perspective this is conceptually flawed – restoring a cached old copy would be correct IMO – although it's a mostly sufficient approximation of the correct approach. If we ever find/implement a good way cache and restore that cached old copy, setting the CWD to the destination where files are being rendered when replaying the fresh copy is more likely to yield the same result because the replayed copy is not affected by the content of the user-specified destination. We already have this side-effect problem with loading external data from the user-specified destination.
WDYT?
There was a problem hiding this comment.
Good point — and I think we're aligned: the chdir lives inside _render_template, so each render's CWD is its own dst_path — the old-copy replay renders with CWD at the old-copy temp dir, the new-copy render at the new-copy temp dir, never the user-specified destination as such. The only reason a.txt showed up in this test is that the current update algorithm's "normal update in final destination" step renders into the real destination, so for that one render the two notions coincide. Under #2376, no render would target the user-specified destination, so fileglob would never see user files — which I agree is the more consistent and predictable semantics (and composes better with restoring a cached old copy per #1170).
So the a.txt assertion was pinning an artifact of the current algorithm rather than intended behavior. I've pushed a commit that drops the user-added file and asserts ['version.txt'] instead, which asserts only the invariant both algorithms share: renders see the destination being rendered, never the invocation directory (the decoy file still guards the original bug from #1708). It should survive the switch to #2376 unchanged, and I left a note in the test explaining why user-added files are deliberately not asserted.
…ing update Whether files added by the user in the destination are visible to filesystem filters during an update depends on whether the update algorithm renders into the user-specified destination, which is an implementation detail that may change (copier-org#2376). Assert only the invariant shared by both algorithms: rendering happens in the destination being rendered, never the invocation directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #1708.
What
While rendering a subproject through
copier {copy,recopy,update}, the working directory was whatever directory Copier was invoked from, so filesystem-related Jinja filters and extensions (e.g. thefileglobfilter from the always-loadedjinja2_ansible_filters.AnsibleCoreFiltersExtension) operated relative to the invocation directory. With this PR, rendering happens with the destination path as the working directory, so the reproducible example from #1708 produces the expected output:Design decisions
Since #1708 ended with "WDYT?", here are the choices made, explicitly:
_render_template(both path-name and content rendering), so it applies tocopy,recopy, andupdate— all three renders of the update flow go throughrun_copy, so the temporary old/new copies render with the working directory at their own temporary destination, and the render into the real destination runs with the working directory there. Tasks and migrations already run with the destination as working directory, so this aligns rendering with the rest of the codebase.run_copy's documented behavior ("Ifdst_pathwas missing, it will be created"); the only observable difference is that a template which renders zero files now still creates the (empty) destination folder.--pretend._external_datanow resolves against the absolute destination path instead of the possibly-relativedst_path, because external data may be loaded lazily during rendering, when the working directory is the destination and a relativedst_pathwould no longer resolve correctly. No behavior change otherwise.jinja_extensions.Tests
copy: working directory is the destination while rendering, with both absolute and relativedst_path, and the invocation directory is restored afterwards (a decoy file next to the invocation directory guards against regressions).update: working directory is the real destination while rendering into it.pretend: a missing destination is not created.dst_path.Full test suite passes locally (1143 passed), as do
mypy,ruff check,ruff format, andcodespell.@sopw686 asked about working on this back in June — apologies if there's any overlap; happy to defer to your PR if you have one in progress.