Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#9426 closed defect (duplicate)

Docbuilder ignores return code from subprocess.call()

Reported by: leif Owned by: mvngu
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: documentation Keywords: Sphinx documentation builder error
Cc: jdemeyer Merged in:
Authors: Reviewers: Minh Van Nguyen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

devel/sage/doc/common/builder.py:

In builder_helper.f():

...
        logger.warning(build_command)
        subprocess.call(build_command, shell=True)

        logger.warning("Build finished.  The built documents can be found in %s", output_dir)
...

In class DocBuilder:

    def pdf(self):
        """
        Builds the PDF files for this document.  This is done by first
        (re)-building the LaTeX output, going into that LaTeX
        directory, and running 'make all-pdf' there.

        EXAMPLES::

            sage: import os, sys; sys.path.append(os.environ['SAGE_DOC']+'/common/'); import builder
            sage: b = builder.DocBuilder('tutorial')
            sage: b.pdf() #not tested
        """
        self.latex()
        os.chdir(self._output_dir('latex'))
        subprocess.call('make all-pdf', shell=True)

        pdf_dir = self._output_dir('pdf')
        for pdf_file in glob.glob('*.pdf'):
            shutil.move(pdf_file, os.path.join(pdf_dir, pdf_file))

        logger.warning("Build finished.  The built documents can be found in %s", pdf_dir)

Change History (12)

comment:1 Changed 11 years ago by leif

It's trivial to change the message on non-zero return values, but I think errors should somehow be propagated, s.t. doc build errors aren't drowned in the flood of other messages.

comment:2 Changed 11 years ago by rlm

  • Priority changed from critical to blocker

Changing this to blocker at least for now...

comment:3 Changed 11 years ago by rlm

  • Priority changed from blocker to major

comment:4 Changed 11 years ago by leif

  • Cc jdemeyer added

comment:5 Changed 11 years ago by leif

  • Keywords Sphinx documentation builder error added

comment:6 follow-up: Changed 11 years ago by jhpalmieri

Should a docbuild error just cause an exception to be raised, thus ending the build? That wouldn't be hard to implement, I think.

comment:7 in reply to: ↑ 6 ; follow-ups: Changed 11 years ago by leif

Replying to jhpalmieri:

Should a docbuild error just cause an exception to be raised, thus ending the build? That wouldn't be hard to implement, I think.

I would have implemented that if I felt it was appropriate. ;-)

An error in a single document should IMHO not stop / break the whole build, but should be propagated and reported (also) at the end, in a summary. Same for warnings, but even harder to implement. (See also #10200.)

I actually do not understand why we do an equivalent of os.system("sphinx ..."); as far as I know, Sphinx is itself written in Python and its functionality should therefore be directly accessible to us from the builder.py script. This way we would easily get the warnings as well.

comment:8 in reply to: ↑ 7 Changed 11 years ago by leif

Replying to leif:

Replying to jhpalmieri:

Should a docbuild error just cause an exception to be raised, thus ending the build? That wouldn't be hard to implement, I think.

I would have implemented that if I felt it was appropriate. ;-)

Looks as if #10191 does exactly that, but only in one of the two cases.

comment:9 Changed 11 years ago by jdemeyer

I was unaware of this ticket and created #10191 (for the case where Sphinx fails catastrophically and aborts) and #10200 (for the case where Sphinx gives a WARNING or ERROR but continues).

comment:10 in reply to: ↑ 7 Changed 11 years ago by jdemeyer

Replying to leif:

An error in a single document should IMHO not stop / break the whole build, but should be propagated and reported (also) at the end, in a summary. Same for warnings, but even harder to implement. (See also #10200.)

This is actually implemented now in #10200 (needs_review).

I propose to close this ticket as duplicate (given #10200).

comment:11 Changed 11 years ago by mvngu

  • Milestone set to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from new to closed

Close as duplicate of #10200.

comment:12 Changed 11 years ago by jdemeyer

  • Reviewers set to Minh Van Nguyen
Note: See TracTickets for help on using tickets.