Closed mgeier closed 1 month ago
Hi @mgeier - I've begun investigating this - can you confirm that what you see is that the relevant CSS files are copied into the build's _static
directory, but that they do not appear as <link href="...">
elements in the rendered HTML (as we'd expect from the documentation of add_css_file
).
If so: I think that may provide some hints about what kind of test coverage we'd want to add (ideally something that passes if tested against ae206694e68bea074aca633ea0d32e9ed882a95f, fails currently, and then we can resolve with a fix)
passes if tested against https://github.com/sphinx-doc/sphinx/commit/ae206694e68bea074aca633ea0d32e9ed882a95f
Sorry: that should be: passes if tested against commits prior to ae206694e68bea074aca633ea0d32e9ed882a95f
Thanks for looking into this, @jayaddison!
can you confirm that what you see is that the relevant CSS files are copied into the build's
_static
directory, but that they do not appear as<link href="...">
elements in the rendered HTML (as we'd expect from the documentation ofadd_css_file
).
I think I didn't describe it clearly enough, let me try again:
The CSS files are copied and the <link href="...">
elements are created fine, but what doesn't work is that a user's static file with the same name doesn't overwrite the one added by the extension.
I guess this has something to do with the added checksum, the actual link looks like this:
<link rel="stylesheet" type="text/css" href="../_static/nbsphinx-gallery.css?v=c0254da5" />
I don't really know how this checksum stuff works, but it somehow seems to inhibit the overwriting with the user-specified CSS file.
Let me try to make a more minimal reproducer:
myext.py
:
import os
import sphinx
def html_collect_pages(app):
sphinx.util.fileutil.copy_asset(
os.path.join(os.path.dirname(__file__), 'myext_static', 'myext.css'),
os.path.join(app.builder.outdir, '_static'))
return []
def setup(app):
app.connect('html-collect-pages', html_collect_pages)
app.add_css_file('myext.css')
myext_static/myext.css
:
h1 { color: red; }
user_static/myext.css
:
h1 { color: green; }
conf.py
:
extensions = ['myext']
html_static_path = ['user_static']
index.rst
:
Hello
=====
The title is green, because the user overrides the extension's CSS file.
Starting with ae206694e68bea074aca633ea0d32e9ed882a95f, the title is red. The user's CSS override is ignored.
I don't really know how this checksum stuff works, but it somehow seems to inhibit the overwriting with the user-specified CSS file.
I'm still learning it, but my reading of ae206694e68bea074aca633ea0d32e9ed882a95f was that one potentially-relevant change is to the event timing (order of operations).
Previously static assets were copied at build-finish and since the change they are copied at an early preparation phase.
From the minimal repro you provided (thank you!) I'm wondering whether this has caused the html-collect-pages
event to be processed in a way that means the overriding file is missed.
I'm taking a break for a while but my next step would be to add your minimal repro as a test case in our tests
dir and then figure out how/why the relevant commit affects the outcome.
None of this is urgent, so please take your time!
But I think you are on the right track: this change in myext.py
seems to fix the problem:
def builder_inited(app):
sphinx.util.fileutil.copy_asset(
os.path.join(os.path.dirname(__file__), 'myext_static', 'myext.css'),
os.path.join(app.builder.outdir, '_static'))
def setup(app):
app.connect('builder-inited', builder_inited)
app.add_css_file('myext.css')
However, I think this is too early for my extension to know which CSS files should be copied. I could of course copy them just in case, but I would prefer not to copy them if they are not needed.
This may or may not be related: with the pytest-randomly
plugin installed, I find that one of our test modules, test_theming.py
has a test case that begins failing at ae206694e68bea074aca633ea0d32e9ed882a95f:
sphinx.git $ pip install pytest==8.1.1
sphinx.git $ pip install pytest-randomly==3.15.0
sphinx.git $ pytest --randomly-seed=0 tests/test_theming.py
...
=========================== short test summary info ============================
FAILED tests/test_theming.py::test_dark_style - assert '<link rel="stylesheet" type="text/css" href="_static/pygments.css?v...
========================= 1 failed, 5 passed in 0.64s ==========================
I would like to know: how can we make it work without breaking 7.2.6? I know that the introduction of checksums had a downstream impact that we did not expect so I would like to reduce as much as possible the risk.
This may be related: when I override html_static_path
it causes a recursive loop. A minimal reproducible example: use sphinx-quickstart
to create a new project, then build it as HTML, via:
sphinx-build -C -D "html_static_path=mytheme," . _build # note the trailing comma
That causes OSError(36, 'Filename too long')
warnings and nothing is copied:
WARNING: Failed to copy a file in html_static_file: ./_build/html/_static/_build/html/_static/_build/html/_static/... etc 4,000-chars long etc .../_build/html/_static/mytheme/simple.css: OSError(36, 'File name too long')
I can see convert_overrides() sets config['html_static_path'] = ['mytheme', '']
, and validate_html_static_path() doesn't strip the empty string item, but haven't dug any deeper yet.
# Platform: linux; (Linux-6.5.0-25-generic-x86_64-with-glibc2.35)
# Sphinx version: 7.2.6
# Python version: 3.10.12 (CPython)
# Docutils version: 0.20.1
# Jinja2 version: 3.1.2
# Pygments version: 2.16.1
@picnixz wrote:
I know that the introduction of checksums had a downstream impact that we did not expect
I don't think that the checksums themselves are the problem here, since it does work perfectly fine when I call it in the builder-inited
event handler (see https://github.com/sphinx-doc/sphinx/issues/12096#issuecomment-2000408306). I didn't check whether the checksums are correct, though.
@jayaddison wrote above (https://github.com/sphinx-doc/sphinx/issues/12096#issuecomment-2000390219) that the timing when assets are copied has also been changed, which might have caused the regression.
This is indeed a timing issue.
html-collect-pages
is emitted during the gen_pages_from_extensions()
finish task, which previously was before copy_{download,static,extra}_files
. ae20669 moved the copy_*_files
tasks to a new builder.copy_assets()
function, called in builder.write
.
This moved copying assets to after env-get-updated
/ env-check-consistency
and before the first doctree-resolved
. Before, assets were copied after html-collect-pages
and before build-finished
.
A
The underlying issue is that as the asset copying has been moved earlier, the extension silently overwrites the files at a now-later step. Perhaps a remedial step would be to add an overwrite=False
parameter to copy_asset
, copy_asset_file
, etc.
Would this work for your use-case @mgeier?
(as a quick fix you could change your hook to env-get-updated
instead of html-collect-pages
, so that your extension writes the files before the user's are copied, but that doesn't fix the silent overwriting issue)
A
I think it would be best if this could be solved within Sphinx itself.
In my case (in nbsphinx
) it is no problem for me to move the copy_asset()
calls to an earlier event callback, because it happens to have no dependencies on the parsing phase (which is dubious in itself, because I shouldn't copy files that are not needed by any source file).
However, I think it is quite confusing to extension authors that the copy_asset()
function has different behavior depending on which event callback it is called in.
If this behavior is kept, there should be at least a warning issued if it is called in a "forbidden" callback.
But is there a logical argument to why copy_asset()
shouldn't be called in html-collect-pages
?
I have the feeling that it should be allowed to be called in any callback (with the same behavior), even in the very last one.
Perhaps a remedial step would be to add an
overwrite=False
parameter tocopy_asset
,copy_asset_file
, etc.
I don't understand how that would help.
An extension should never overwrite a file provided by the site author, right? So there is no need to provide a choice for the extension author. As far as I'm concerned, an extension author shouldn't even need to know that their files can be overwritten by site authors.
A
@AA-Turner Thanks for changing the behavior (#12647) and making wrong usage more obvious (#12612)!
(as a quick fix you could change your hook to
env-get-updated
instead ofhtml-collect-pages
, so that your extension writes the files before the user's are copied, but that doesn't fix the silent overwriting issue)
For the record, I have moved it into builder-inited
in https://github.com/spatialaudio/nbsphinx/pull/808.
Describe the bug
In my concrete case, the CSS file is added with
add_css_file()
in mynbsphinx
extension: https://github.com/spatialaudio/nbsphinx/blob/53be5e632f0c9483ba65ba6690da9b137f9c1120/src/nbsphinx/__init__.py#L1655How to Reproduce
Check out https://github.com/spatialaudio/nbsphinx/ and add this to
doc/conf.py
:Create a file
my-static-path/nbsphinx-gallery.css
with some CSS overrides, like e.g. in https://github.com/Substra/substra-documentation/blob/d2785aa82d46686d82ab66d2c24f95af86d28ce6/docs/source/static/nbsphinx-gallery.css.Run Sphinx.
Look at one of the thumbnail galleries.
Environment Information
Sphinx extensions
Additional context
This was originally reported here: https://github.com/spatialaudio/nbsphinx/issues/778
I have bisected it, and the culprit is ae206694e68bea074aca633ea0d32e9ed882a95f, which is part of #11415.