Regression: CSS file added by extension cannot be overridden by users anymore

[mgeier]

Describe the bug

In my concrete case, the CSS file is added with add_css_file()
in my nbsphinx extension: https://github.com/spatialaudio/nbsphinx/blob/53be5e632f0c9483ba65ba6690da9b137f9c1120/src/nbsphinx/__init__.py#L1655

How to Reproduce

Check out https://github.com/spatialaudio/nbsphinx/ and add this to doc/conf.py:

html_static_path = ['my-static-path']

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

Any Sphinx version after ae206694e68bea074aca633ea0d32e9ed882a95f, appearing first in release 7.1.0.

Sphinx extensions

`nbsphinx`, but this should happen with any extension that adds some CSS.

Additional context

This was originally reported here: spatialaudio/nbsphinx#778

I have bisected it, and the culprit is ae20669, which is part of #11415.

[jayaddison]

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 ae20669, fails currently, and then we can resolve with a fix)

[jayaddison]

passes if tested against ae20669

Sorry: that should be: passes if tested against commits prior to ae20669

[mgeier]

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 of add_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.

[mgeier]

Let me try to make a more minimal reproducer:

Extension

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's Project

user_static/myext.css:

h1 { color: green; }

conf.py:

extensions = ['myext']
html_static_path = ['user_static']

index.rst:

Hello
=====

Expected Behavior

The title is green, because the user overrides the extension's CSS file.

Actual Behavior

Starting with ae20669, the title is red. The user's CSS override is ignored.

[jayaddison]

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 ae20669 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.

[jayaddison]

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.

[mgeier]

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.

[jayaddison]

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 ae20669:

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 ==========================

[picnixz]

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.

[explode]

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

[mgeier]

@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 #12096 (comment)). I didn't check whether the checksums are correct, though.

@jayaddison wrote above (#12096 (comment)) that the timing when assets are copied has also been changed, which might have caused the regression.

[AA-Turner]

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

[AA-Turner]

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

[mgeier]

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 to copy_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.

[AA-Turner]

#12612 makes this problem more obvious, but does not fully resolve it. We should make overwriting explicit in Sphinx 8.

A

[mgeier]

@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 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)

For the record, I have moved it into builder-inited in spatialaudio/nbsphinx#808.