-
Notifications
You must be signed in to change notification settings - Fork 394
bugfix: resolve engines before ignoreDirs
#13798
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
bugfix: resolve engines before ignoreDirs
#13798
Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
this data is no longer static and needs to consider engine extensions as well website and default project tests Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| - ([#13633](https://github.com/quarto-dev/quarto-cli/issues/13633)): Fix detection and auto-installation of babel language packages from newer error format that doesn't explicitly mention `.ldf` filename. | ||
| - ([#13694](https://github.com/quarto-dev/quarto-cli/issues/13694)): Fix `notebook-view.url` being ignored - external notebook links now properly use specified URLs instead of local preview files. | ||
| - ([#13732](https://github.com/quarto-dev/quarto-cli/issues/13732)): Fix automatic font package installation for fonts with spaces in their names (e.g., "Noto Emoji", "DejaVu Sans"). Font file search patterns now match both with and without spaces. | ||
| - ([#13798](https://github.com/quarto-dev/quarto-cli/pull/13798)): Directories specified in `ExecutionEngineDiscovery.ignoreDirs` were not getting ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gordonwoodhull I don't think this is the right place for this.
Regression fixes in 1.9 changelog are the bug fix we do in 1.9 AND also in 1.8 to fix a regression that we introduced in 1.8 compared to 1.9.
For regression between a 1.9 pre-release, and another, we don't add changelog. It is in a way expected that a 1.9 pre-release could brok. So this PR is part of the "New engine extensions" item you added a bit lower.
Hope it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cderv, I was wondering as I did this. Felt weird not to log what the prerelease 1.9.14 will be about!
But I guess it's probably not helpful except for the very few people for whom 1.9.13 is breaking - and we haven't heard from any yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess it's probably not helpful except for the very few people for whom 1.9.13 is breaking - and we haven't heard from any yet.
yes exactly. We don't have a good process for that.
I guess the right place would to mark this inside the 1.9.14 release note. Currently we only link users to the commit logs if you look at it: https://github.com/quarto-dev/quarto-cli/releases/tag/v1.9.13
We could find a new workflow logic to maybe add to this github release note some more information about what is noticeable in each pre-release. But this is a bit more work for us.
Unless we find an "automated way" by having specific keyword in commit, and looking for them to extract message for release not when we do create the github release 🤷♂️
Anyhow, our current logic is that there is no release note of specific changes for a specific prerelease (unless looking at commit themselves), and we advice all users to either use stable or very latest pre-release as it could be "normal" that a pre-release has breakage.
For no breakage => use stable. That is our logic I would say.
happy to evolve all this - to be discussed!
The set of engines is no longer static / initialized at module load time.
Therefore
resolveEngines()needs to be called beforeexecutionEngines(). 1This introduces tests for
ignoreDirsof knitr and jupyter engines, which were not being ignored, e.g. causing render of quarto-web to fail404.ipynband404.qmdfiles, which was succeeding but wasn't already testedindex.ipynbandindex.rmdfiles to section links in the sidebar. Engines were already resolved before this, but the feature was untested and it relies on the engine discovery interface, so here is a test.Footnotes
Could there be a better design? There probably could, but
resolveEngines()needs a project context andexecutionEngines()does not, so we can't just make the latter dependent on the former. ↩