Add a test which fails on sphinx warning/failure #11502
Conversation
tests/checks/check-sphinx.fish
Outdated
| @@ -0,0 +1,31 @@ | |||
| #RUN: fish=%fish %fish %s | |||
| #REQUIRES: sphinx-build --help | |||
There was a problem hiding this comment.
I think it would be enough to add it to one of the CI jobs, because it should be mostly architecture-independent:
When I try that, using:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index f300840a97..9f7c01cb87 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -19,7 +19,7 @@
- uses: dtolnay/rust-toolchain@1.70
- name: Install deps
run: |
- sudo apt install gettext libpcre2-dev python3-pexpect tmux
+ sudo apt install gettext libpcre2-dev python3-pexpect python3-sphinx tmux
# Generate a locale that uses a comma as decimal separator.
sudo locale-gen fr_FR.UTF-8
- name: cmake
diff --git a/doc_src/cmds/bind.rst b/doc_src/cmds/bind.rst
index 68e47717ca..c85d1b29de 100644
--- a/doc_src/cmds/bind.rst
+++ b/doc_src/cmds/bind.rst
@@ -426,7 +426,7 @@
bind ctrl-g 'git diff' repaint
Swap :kbd:`tab` and :kbd:`shift-tab`, making tab focus the search field.
-But if the search field is already active, keep the behavior (:kbd:`tab` cycles forward, :kbd:`shift-tab` backward).::
+But if the search field is already active, keep the behavior (:kbd:`tab` cycles forward, :kbd:`shift-tab` backward).
bind tab '
if commandline --search-field >/dev/nullI get
checks/check-sphinx.fish..Failure:
There were no remaining checks left to match stderr:1:
usage: sphinx-build [OPTIONS] SOURCEDIR OUTPUTDIR [FILENAMES...]
Context:
usage: sphinx-build [OPTIONS] SOURCEDIR OUTPUTDIR [FILENAMES...] <= no more checks
sphinx-build: error: unrecognized arguments: --quiet --fail-on-warning --fresh-env --builder --conf-dir --doctree-dir /tmp/fishtest-cwgh_0pj/temp/tmp.BN9qwuRnmy/doctree /home/runner/work/fish-shell/fish-shell/tests/checks/../../doc_src /tmp/fishtest-cwgh_0pj/temp/tmp.BN9qwuRnmy/man
usage: sphinx-build [OPTIONS] SOURCEDIR OUTPUTDIR [FILENAMES...]
sphinx-build: error: unrecognized arguments: --quiet --fail-on-warning --fresh-env --builder --conf-dir --doctree-dir /tmp/fishtest-cwgh_0pj/temp/tmp.BN9qwuRnmy/doctree /home/runner/work/fish-shell/fish-shell/tests/checks/../../doc_src /tmp/fishtest-cwgh_0pj/temp/tmp.BN9qwuRnmy/html
see https://github.com/krobelus/fish-shell/actions/runs/15061701853/job/42337871529
probably the sphinx version on GHA is too old? Maybe get a pinned version with pip3 / pipx?
In future, we should probably stop pinning to get free auto-updates (unless a bot can do the updates).
Of course that means that we'll want to fix any warnings after each sphinx upgrades, let's hope that's not a frequent issue.
There was a problem hiding this comment.
Apparently many of the long options have been added fairly recently. I changed it to short options.
There was a problem hiding this comment.
I think we should only merge this if it's checked in CI - can you add that?
If it causes annoyance, we can remove it but I think it should be fine.
The only other thing we might want to decide is how/when to update cargo expand (but we may be able to defer this decision, see the next paragraph). I see two usable approaches
- always use the latest available on the github actions runner (on stable rust, not 1.70)
Note that our MSRV is still 1.70 but most users will not need to generate translations. - pin the version in the CI, and ideally add a bot that auto-updates the cargo version
However, we can probably move away from using cargo expand.
I was told about https://github.com/Aleph-Alpha/ts-rs
which does something similar (they extract typescript models for structs annotated with a proc macro).
Look for the export_bindings test.
We could do something similar, i.e. create such a test that dumps all translatable strings.
If you're up for that that would be very welcome.
I still need to get around to reviewing all your other patches :)
Of course we should still figure out a convention on when to update dependencies
(using a bot, or do it once a year etc) and maybe at least document that.
There was a problem hiding this comment.
sorry in the above comment everything beyond the first paragraph obviously applies only to #11497
There was a problem hiding this comment.
I think we should only merge this if it's checked in CI - can you add that?
I didn't realize that it was being skipped. Adding the python3-sphinx package fixed that, but the test now fails because sphinx uses fish_indent which does not make it into $PATH. From my current understanding this is because littlecheck does not actually modify $PATH, but instead replaces commands in the #REQUIRES: ... lines with paths to the cmake build directory, which of course has no effect on scripts called from such tests.
What do you think is the best way to address this? One way to do it would be to do something similar to the translation update check and just run it separately from the normal tests, where we can set $PATH directly.
There was a problem hiding this comment.
it's preferable that running this test locally uses fish_indent from the git checkout, and not /bin/fish_indent.
(Technically this also holds for the CI translations check... but I would be very surprised if we ever made build_tools/update_translations.fish incompatible with /bin/fish so I guess we don't care for now)
So I think we should put it in the test, not in the CI config, maybe:
#RUN: fish_indent=%fish_indent fish=%fish %fish %s
...
set -lxp PATH (path dirname $fish_indent)
sphinx-build ...
endor equivalently PATH=(path dirname $fish_indent):$PATH sphinx-build ...
Of course this also injects fish and fish_key_reader but sphinx should never run those.
There was a problem hiding this comment.
This seems to work. I think it makes sense to use a similar approach for checking the translations when we no longer need cargo-expand via #11536. It seems like tests run in parallel in CI, so we should probably add the PO file update check before the existing format check in check-translations. It might make sense to skip the PO update check if there are uncommitted changed in po/, and maybe print a warning instead. That's not relevant for CI, but could prevent mixing manual translation updates with automated changes.
There was a problem hiding this comment.
we should probably add the PO file update check before the existing format check in check-translations
yeah, probably in the same test file.
It might make sense to skip the PO update check if there are uncommitted changed in po/, and maybe print a warning instead
My first thought is: if a test runs update_translations.fish, it could output to a temporary directory and then run
diff -ur po /tmp/fish-update-translations.123/.
A test should not write source files or know anything about Git.
Either way, if one doesn't want to re-run update_translations.fish, they can pipe the test output into git apply.
If the diff -ur output doesn't work out of the box we can make it look like git diff. But that's probably not super important.
Of course we could have a command like ninja -Cbuild generate that runs cargo fmt, update_translations.fish etc, and errors out if it would overwrite uncommitted changes.
There was a problem hiding this comment.
I opened #11542 to prepare the update_translations.fish script for running in a test. When we no longer need cargo-expand, the special handling in CI can be deleted and replaced by a normal test, which can also run locally without modifying the PO files (aside from template.po).
tests/checks/check-sphinx.fish
Outdated
| @@ -0,0 +1,31 @@ | |||
| #RUN: fish=%fish %fish %s | |||
| #REQUIRES: sphinx-build --help | |||
There was a problem hiding this comment.
This test need not depend on fish so it need not be in tests/checks/ but I guess it's okay.
The advantage of fitting into the tests/checks/ format is that it's understood by both cmake as well as our standalone test driver, i.e.
cargo b && tests/test_driver.py target/debug tests/checks/check-sphinx.fish
the alternative would be to make the test a standalone script (and add it to the fish_run_tests target).
I don't think it matters since this is an edge case.
There was a problem hiding this comment.
I basically just copied the way it is done for checking the po files in check-translations.fish. I don't really care about the location, but I think it is good to run the tests locally along with the other tests, so people can spot issues more easily before they hit CI.
I don't like the idea of custom, CMake-dependent handling.
What we could also consider is adding the -W (fail on warning) flag in build.rs. This would make fish builds fail on sphinx warnings. Without -E (fresh env, complete rebuild) the failures would be ignored on subsequent builds, but build times would be longer if we add -E.
e40a7e6 to
e1e8634
Compare
e1e8634 to
2835c5a
Compare
2835c5a to
c2e2237
Compare
TODOs: