Skip to content
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

docbuild errors out with "cannot copy static file FileExistsError" #32262

Closed
kwankyu opened this issue Jul 22, 2021 · 36 comments
Closed

docbuild errors out with "cannot copy static file FileExistsError" #32262

kwankyu opened this issue Jul 22, 2021 · 36 comments

Comments

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 22, 2021

There is apparently a race condition in docbuild which leads to

  [dochtml]   [reference]     topology: 1937 js index entries
  [dochtml]   [reference]     valuations: 973 js index entries
  [dochtml]   [reference] ... done (64387 js index entries)
  [dochtml]   [reference] copying static files... failed
  [dochtml]   [reference] WARNING: cannot copy static file FileExistsError(17, 'File exists')
  [dochtml]   [reference] dumping search index in English (code: en)... done
  [dochtml]   [reference] The HTML pages are in .tox/local-homebrew-macos-standard/local/share/doc/sage/html/en/reference.
  [dochtml]   Error building the documentation.
  [dochtml]   Traceback (most recent call last):
  [dochtml]     File "/Users/runner/work/sagetrac-mirror/sagetrac-mirror/.tox/local-homebrew-macos-standard/homebrew/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 197, in _run_module_as_main
  [dochtml]       return _run_code(code, main_globals, None,
  [dochtml]     File "/Users/runner/work/sagetrac-mirror/sagetrac-mirror/.tox/local-homebrew-macos-standard/homebrew/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 87, in _run_code
  [dochtml]       exec(code, run_globals)
  [dochtml]     File "/Users/runner/work/sagetrac-mirror/sagetrac-mirror/.tox/local-homebrew-macos-standard/local/lib/python3.9/site-packages/sage_docbuild/__main__.py", line 2, in <module>
  [dochtml]       main()
  [dochtml]     File "/Users/runner/work/sagetrac-mirror/sagetrac-mirror/.tox/local-homebrew-macos-standard/local/lib/python3.9/site-packages/sage_docbuild/__init__.py", line 1813, in main
  [dochtml]       builder()
  [dochtml]     File "/Users/runner/work/sagetrac-mirror/sagetrac-mirror/.tox/local-homebrew-macos-standard/local/lib/python3.9/site-packages/sage_docbuild/__init__.py", line 133, in f
  [dochtml]       runsphinx()
  [dochtml]     File "/Users/runner/work/sagetrac-mirror/sagetrac-mirror/.tox/local-homebrew-macos-standard/local/lib/python3.9/site-packages/sage_docbuild/sphinxbuild.py", line 323, in runsphinx
  [dochtml]       sys.stderr.raise_errors()
  [dochtml]     File "/Users/runner/work/sagetrac-mirror/sagetrac-mirror/.tox/local-homebrew-macos-standard/local/lib/python3.9/site-packages/sage_docbuild/sphinxbuild.py", line 258, in raise_errors
  [dochtml]       raise OSError(self._error)
  [dochtml]   OSError: WARNING: cannot copy static file FileExistsError(17, 'File exists')
  [dochtml]   
  [dochtml]       Note: incremental documentation builds sometimes cause spurious
  [dochtml]       error messages. To be certain that these are real errors, run
  [dochtml]       "make doc-clean" first and try again.
  [dochtml]   make[3]: *** [doc-html--reference_top] Error 1
  [dochtml]   make[2]: *** [doc-html-reference] Error 2
  [dochtml]   make[2]: Target `doc-html' not remade because of errors.
  [dochtml] Full log file: /Users/runner/work/sagetrac-mirror/sagetrac-mirror/logs/dochtml.log
make[1]: *** [doc-html] Error 2

on many GH Action runs on #31580, e.g.
https://github.com/sagemath/sagetrac-mirror/runs/3104375540 (this is docker (ubuntu-bionic, standard))
https://github.com/sagemath/sagetrac-mirror/runs/3104375098 (this is local-macos (homebrew-macos-python3_xcode, standard, default, macos-11.0))

also reproducible locally on macOS/Homebrew with make -j8.

No failure with make -j1.

This is observed with the latest beta on Linux and macOS after #31948

Reported on sage-dev: https://groups.google.com/g/sage-devel/c/ts6u19KpapA/m/22BdxXZhBAAJ

Depends on #31580

Upstream: Fixed upstream, but not in a stable release.

CC: @jhpalmieri @isuruf

Component: build

Issue created by migration from https://trac.sagemath.org/ticket/32262

@kwankyu kwankyu added this to the sage-9.4 milestone Jul 22, 2021
@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 22, 2021

comment:1

This is just a gut feeling but I suspect the issue is related with the option --no-prune-empty-dirs in these lines

+	$(MAKE) SAGE_DOCBUILD_OPTS="$(SAGE_DOCBUILD_OPTS) --no-prune-empty-dirs" $(foreach doc, $(OTHER_DOCS), doc-inventory--$(subst /,-,$(doc)))
+	$(MAKE) SAGE_DOCBUILD_OPTS="$(SAGE_DOCBUILD_OPTS) --no-prune-empty-dirs" doc-inventory--reference_top

The other issue in #32043 may be related with this.

@kwankyu

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Jul 22, 2021

comment:3

git grep -i static src/sage_docbuild/ reveals several places that would be worth inspecting.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 22, 2021

comment:4

This is where the error is raised: in sphinx/builders/html/__init__.py

    def copy_html_static_files(self, context: Dict) -> None:
        def onerror(filename: str, error: Exception) -> None:
            logger.warning(__('Failed to copy a file in html_static_file: %s: %r'),
                           filename, error)

        excluded = Matcher(self.config.exclude_patterns + ["**/.*"])
        for entry in self.config.html_static_path:
            copy_asset(path.join(self.confdir, entry),
                       path.join(self.outdir, '_static'),
                       excluded, context=context, renderer=self.templates, onerror=onerror)

    def copy_html_logo(self) -> None:
        if self.config.html_logo and not isurl(self.config.html_logo):
            copy_asset(path.join(self.confdir, self.config.html_logo),
                       path.join(self.outdir, '_static'))

    def copy_html_favicon(self) -> None:
        if self.config.html_favicon and not isurl(self.config.html_favicon):
            copy_asset(path.join(self.confdir, self.config.html_favicon),
                       path.join(self.outdir, '_static'))

    def copy_static_files(self) -> None:
        try:
            with progress_message(__('copying static files')):
                ensuredir(path.join(self.outdir, '_static'))

                # prepare context for templates
                context = self.globalcontext.copy()
                if self.indexer is not None:
                    context.update(self.indexer.context_for_searchtool())

                self.create_pygments_style_file()
                self.copy_translation_js()
                self.copy_stemmer_js()
                self.copy_theme_static_files(context)
                self.copy_html_static_files(context)
                self.copy_html_logo()
                self.copy_html_favicon()
        except OSError as err:
            logger.warning(__('cannot copy static file %r'), err)  

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 22, 2021

comment:5

What is the command to reproduce the bug?

@dimpase
Copy link
Member

dimpase commented Jul 22, 2021

comment:8

Replying to @kwankyu:

What is the command to reproduce the bug?

try building the branch of #31580 with make -jX (for X=8, say?) on macOS or Ubuntu.

I've opened a duplicate ticket (oops) #32263 with a bit more info.

@kwankyu

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:10

This seems tied to #31580: I can only reproduce it with that branch. (I've also tried with the two dependencies for #31580, individually and together, but I don't get this error.) I don't know why upgrading matplotlib would have this effect on docbuilding, though.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 26, 2021

comment:11

Investigating this, I find somethings strange.

My sage uses system python 3.9 but Sphinx building the doc uses sage's own python 3.9. Is this normal? How can I run manually sage's own python?

The bug appeared after make doc-clean but after a few(or one?) make -j8, it disappeared.

#27754 made special tweaks for mac regarding parallel building. Might be related...

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 26, 2021

comment:12

Replying to @jhpalmieri:

This seems tied to #31580: I can only reproduce it with that branch. (I've also tried with the two dependencies for #31580, individually and together, but I don't get this error.) I don't know why upgrading matplotlib would have this effect on docbuilding, though.

Right. Now I saw the crawling bug. Somehow matplotlib writes a file named _static with content

/*
 * plot_directive.css
 * ~~~~~~~~~~~~
 *
 * Stylesheet controlling images created using the `plot` directive within
 * Sphinx.
 *
 * :copyright: Copyright 2020-* by the Matplotlib development team.
 * :license: Matplotlib, see LICENSE for details.
 *
 */

img.plot-directive {
    border: 0;
    max-width: 100%;
}

into the directory local/share/doc/sage/html/en/reference. Later when Sphinx tries to create a directory named _static (into which static html-related files are copied) in the same place, it booms with FileExistsError (essentially a name clash). Someone should explain how and why matplotlib write the file _static there.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 26, 2021

comment:13

In

https://matplotlib.org/stable/_modules/matplotlib/sphinxext/plot_directive.html

Find

def _copy_css_file(app, exc):
    if exc is None and app.builder.format == 'html':
        src = cbook._get_data_path('plot_directive/plot_directive.css')
        dst = app.outdir / Path('_static')
        shutil.copy(src, dst)

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 26, 2021

comment:14

Ah, hence Sphinx needs to create _static directory before matplotlib does..

@jhpalmieri
Copy link
Member

comment:15

I think that this function is called by Sphinx+matplotlib when it invokes (or uses?) the plot_directive extension in src/sage/docs/conf.py (https://github.com/sagemath/sage-prod/blob/develop/src/sage/docs/conf.py). See in particular line 26 and the sphinx_plot function.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 27, 2021

comment:16

Now I see that the issue of this ticket is caused by abandoning AllBuilder in sage_docbuild/__init__.py when #31948 made a move to make-based docbuilding. The class AllBuilder creates the necessary _static directory, but make does not do it. See this

class AllBuilder(object):
    """
    A class used to build all of the documentation.
    """
    ...
    def _wrapper(self, name, *args, **kwds):
        """
        This is the function which goes through all of the documents
        and does the actual building.
        """
        ...
        logger.warning("Building reference manual, second pass.\n")
        sage_makedirs(os.path.join(SAGE_DOC, "html", "en", "reference", "_static"))
        for document in refs:
            getattr(get_builder(document), name)(*args, **kwds)   

I think the issue of #32043 is similarly caused by #31948 by not invoking ReferenceTopBuilder in the make-based docbuilding.

I leave the necessary work to fix these to the experts on the make files.

@dimpase
Copy link
Member

dimpase commented Jul 27, 2021

comment:17

My experiments show the opposite: if creating _static explicitly is not done, i.e

--- a/src/sage_docbuild/__init__.py
+++ b/src/sage_docbuild/__init__.py
@@ -345,7 +345,7 @@ class AllBuilder(object):
             getattr(get_builder(document), 'inventory')(*args, **kwds)
 
         logger.warning("Building reference manual, second pass.\n")
-        sage_makedirs(os.path.join(SAGE_DOC, "html", "en", "reference", "_static"))
+        # sage_makedirs(os.path.join(SAGE_DOC, "html", "en", "reference", "_static"))
         for document in refs:
             getattr(get_builder(document), name)(*args, **kwds)

then docs build. Indeed, in the big picture doing things in the docs directory should be left to sphinx as much as possible.
matplotlib creates _static via a sphinx extension, so this should be OK, as opposed to brute-forcing it like it's done in that line.

@dimpase
Copy link
Member

dimpase commented Jul 27, 2021

comment:18

oops, no, I take comment:17 back. The patch there has no effect.

On the other hand, I really don't see why the builder should explicitly mess around with the sphinx's directory.

@jhpalmieri
Copy link
Member

comment:19

I think the fault lies with matplotlib: in the code in comment:13, they assume that the _static directory exists. When it doesn't, then at least on my machine, it creates a file _static containing the plot_directive content, rather than creating _static/plot_directive.css.

The following (where we forcibly create the _static directory when building any sub-component of the reference manual before matplotlib does anything) seems to fix it for me:

diff --git a/src/sage_docbuild/__init__.py b/src/sage_docbuild/__init__.py
index b63e53539e..41140f4e1e 100644
--- a/src/sage_docbuild/__init__.py
+++ b/src/sage_docbuild/__init__.py
@@ -783,6 +783,7 @@ class ReferenceSubBuilder(DocBuilder):
         """
         # Force regeneration of all modules if the inherited
         # and/or underscored members options have changed.
+        sage_makedirs(os.path.join(SAGE_DOC, "html", "en", "reference", "_static"))
         cache = self.get_cache()
         force = False
         try:

@dimpase
Copy link
Member

dimpase commented Jul 27, 2021

comment:20

Replying to @jhpalmieri:

I think the fault lies with matplotlib: in the code in comment:13, they assume that the _static directory exists. When it doesn't, then at least on my machine, it creates a file _static containing the plot_directive content, rather than creating _static/plot_directive.css.

The following (where we forcibly create the _static directory when building any sub-component of the reference manual before matplotlib does anything) seems to fix it for me:

diff --git a/src/sage_docbuild/__init__.py b/src/sage_docbuild/__init__.py
index b63e53539e..41140f4e1e 100644
--- a/src/sage_docbuild/__init__.py
+++ b/src/sage_docbuild/__init__.py
@@ -783,6 +783,7 @@ class ReferenceSubBuilder(DocBuilder):
         """
         # Force regeneration of all modules if the inherited
         # and/or underscored members options have changed.
+        sage_makedirs(os.path.join(SAGE_DOC, "html", "en", "reference", "_static"))
         cache = self.get_cache()
         force = False
         try:

yes, looks like an upstream bug. Would it be better to patch matplotlib?
Anyway, I've left them a comment: matplotlib/matplotlib@25b5ead#r54046428

@jhpalmieri
Copy link
Member

comment:21

Thank you for reporting that upstream.

I prefer avoiding patches to other packages if possible. If there ends up being a matplotlib fix, we can backport that. I also think that the change in comment:19 in how we use Sphinx won't hurt.

@jhpalmieri
Copy link
Member

comment:22

Should I create a branch with my proposed change?

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 28, 2021

comment:23

Replying to @dimpase:

oops, no, I take comment:17 back. The patch there has no effect.

On the other hand, I really don't see why the builder should explicitly mess around with the sphinx's directory.

As far as I understand, sphinx builder (StandaloneHTMLBuilder in sphinx/builders/html/__init__.py) creates the _static directory. As Sage uses a custom builder, it is the builder's duty to create the directory.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 28, 2021

comment:24

Replying to @jhpalmieri:

Should I create a branch with my proposed change?

ReferenceSubBuilder is concerned in processing subcomponents of the reference manual. I don't think it is the right place to care about creating _static directory.

The make files have taken the job of AllBuilder, which did the work of creating the directory. Then why not the make files take over the work?

@jhpalmieri
Copy link
Member

comment:25

Replying to @kwankyu:

Replying to @jhpalmieri:

Should I create a branch with my proposed change?

ReferenceSubBuilder is concerned in processing subcomponents of the reference manual. I don't think it is the right place to care about creating _static directory.

The make files have taken the job of AllBuilder, which did the work of creating the directory. Then why not the make files take over the work?

Everything needs to be tested, but someone could want to build only en/reference/topology without building the whole reference manual. Or do that and then build the whole manual later, by which point it's too late to create _static: any part of the reference manual which uses the matplotlib plot_directive extension will lead to this problem, so I think we need to address it at a pretty low level.

We didn't run into this before because it's a new Sphinx extension which happens to have a bug (now with a fix at matplotlib/matplotlib#20748).

@jhpalmieri
Copy link
Member

comment:26

Or maybe we did run into it before: I remember years ago having problems with the graphviz Sphinx extension and the _static directory, and I wonder if it was the same thing.

@jhpalmieri
Copy link
Member

comment:27

Replying to @jhpalmieri:

Replying to @kwankyu:

Replying to @jhpalmieri:

Should I create a branch with my proposed change?

ReferenceSubBuilder is concerned in processing subcomponents of the reference manual. I don't think it is the right place to care about creating _static directory.

The make files have taken the job of AllBuilder, which did the work of creating the directory. Then why not the make files take over the work?

Everything needs to be tested, but someone could want to build only en/reference/topology without building the whole reference manual. Or do that and then build the whole manual later, by which point it's too late to create _static: any part of the reference manual which uses the matplotlib plot_directive extension will lead to this problem, so I think we need to address it at a pretty low level.

Also, people may invoke docbuilding using sage --docbuild ... rather than make doc, so I think we have to address it outside of the makefile.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 28, 2021

comment:28

Replying to @jhpalmieri:

Replying to @jhpalmieri:

Replying to @kwankyu:

Replying to @jhpalmieri:

Should I create a branch with my proposed change?

ReferenceSubBuilder is concerned in processing subcomponents of the reference manual. I don't think it is the right place to care about creating _static directory.

The make files have taken the job of AllBuilder, which did the work of creating the directory. Then why not the make files take over the work?

Everything needs to be tested, but someone could want to build only en/reference/topology without building the whole reference manual. Or do that and then build the whole manual later, by which point it's too late to create _static: any part of the reference manual which uses the matplotlib plot_directive extension will lead to this problem, so I think we need to address it at a pretty low level.

Also, people may invoke docbuilding using sage --docbuild ... rather than make doc, so I think we have to address it outside of the makefile.

Isn't it the initial make-ing of Sage that also includes docbuilding that we now try to fix? After successful initial make, there is no problem.

@jhpalmieri
Copy link
Member

comment:29

If someone does make build and then sage --docbuild ..., it should work.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 28, 2021

comment:30

Replying to @jhpalmieri:

If someone does make build and then sage --docbuild ..., it should work.

Okay. Then I have no better idea where the fix should be placed.

@dimpase
Copy link
Member

dimpase commented Jul 28, 2021

Upstream: Fixed upstream, but not in a stable release.

@dimpase
Copy link
Member

dimpase commented Jul 28, 2021

comment:31

To fix this particular issue I'm going to add the patch matplotlib/matplotlib#20748 already produced by Matplotlib in #31580

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 28, 2021

comment:32

Replying to @dimpase:

To fix this particular issue I'm going to add the patch matplotlib/matplotlib#20748 already produced by Matplotlib in #31580

Good. Then there's nothing to do here.

@jhpalmieri
Copy link
Member

comment:33

I think that conda builds its own matplotlib. If this is right, then until this fix is included in matplotlib, we either shouldn't merge #31580 or we should accept that the documentation won't build in parallel on conda.

@dimpase
Copy link
Member

dimpase commented Jul 28, 2021

comment:34

I've left a comment for Isuru on matplotlib/matplotlib#20748

@mkoeppe
Copy link
Member

mkoeppe commented Aug 1, 2021

Dependencies: #31580

@mkoeppe mkoeppe removed this from the sage-9.4 milestone Aug 1, 2021
@mkoeppe
Copy link
Member

mkoeppe commented Sep 7, 2021

comment:36

matplotlib 3.4.3 is out and appears to have the fix (matplotlib/matplotlib#20751)

@jhpalmieri
Copy link
Member

comment:37

Let's close this, then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants