Add 'Builder.epilog' attribute by stephenfin · Pull Request #4354 · sphinx-doc/sphinx

stephenfin · 2017-12-28T21:11:26Z

Subject: Add epilog attribute to sphinx.builder.Builder

Feature or Bugfix

Feature

Purpose

The Makefile, make.bat, and sphinx-build -M (make mode) tools all output very similar messages on successful build. Reduce this duplication (and the need for the latter) by moving this into the builders themselves.

Detail

builders: Add Builder.epilog option

This allows builders to emit a final epilog message containing information such as where resulting files can be found. This is only emitted if the build was successful.

This allows us to remove this content from the make_mode tool and the legacy Makefile and make.bat templates. There's room for more dramatic simplification of the former, but this will come later.
make_mode: Remove unnecessary make_* functions

These are handled by the default case.
Makefile: Remove unnecessary targets

Most of these are not necessary now that we're not printing different messages for various builders.
make.bat: Remove unnecessary targets

As with the Makefile previously, these are not necessary now that we're not printing anything different for various builders.
Makefile: Make SOURCEDIR configurable

It's unlikely that anyone will need to do this but at least give them the opportunity.
make.bat: Make SOURCEDIR configurable

It's unlikely that anyone will need to do this but at least give them the opportunity.

stephenfin · 2017-12-28T21:14:53Z

sphinx/application.py

                logger.info(bold(__('build %s.') % status))
+
+            if self.statuscode == 0 and self.builder.epilog:
+                # TODO(stephenfin): Should this use logging or print?


I would like your thoughts on both of these items, @tk0miya 😄

Please use logging module to output messages, warnings and errors.

jfbu · 2017-12-28T22:11:52Z

sphinx/builders/texinfo.py

    format = 'texinfo'
+    epilog = 'The Texinfo files are in %(outdir)s.'
+    if os.name == 'posix':
+        epilog += ("Run 'make' in that directory to run these through "


a space (or rather a \n) is missing at start of string

tk0miya · 2017-12-29T06:47:29Z

sphinx/application.py

                logger.info(bold(__('build %s.') % status))
+
+            if self.statuscode == 0 and self.builder.epilog:
+                # TODO(stephenfin): Should this use logging or print?


Please use logging module to output messages, warnings and errors.

tk0miya · 2017-12-29T06:49:06Z

sphinx/application.py

+                # TODO(stephenfin): Should this use logging or print?
+                logger.info('')
+                # TODO(stephenfin): Should this be printf-based or
+                # .format-based?


Either is fine. Personally, I preferred printf-based one.

OK, I'll stick with printf. None of the disadvantages I can see with printf would affect us here.

tk0miya · 2017-12-29T14:39:22Z

sphinx/application.py

            else:
                logger.info(bold(__('build %s.') % status))
+
+            if self.statuscode == 0 and self.builder.epilog:


What do you think about moving this to the end of Builder.build()? I think emitting epilog message is responsibility of builder itself.

In addition, I'm worried about epilog is better way or not. I think a method of builder is also good like Builder.show_finish_message().

What do you think about moving this to the end of Builder.build()? I think emitting epilog message is responsibility of builder itself.

I can do this. The reason I kept it separate was I wasn't sure if Builder.build() was ever overridden by subclasses. If it was, we could end up emitting the message before any subclasses were called. For example:

class MySpecialBuilder(Builder): ... def build(self, docnames, summary=None, method='update'): super(MySpecialBuilder, self).build(docnames, summary, method) # do something else here

It's your call though. Let me know what you'd prefer.

In addition, I'm worried about epilog is better way or not. I think a method of builder is also good like Builder.show_finish_message().

I prototyped with both a function and a property but both seemed overkill. The logic is always the same: if the build succeeds then emit the message. Using printf formatting will also ensure we get consistent builder messages. Is there any reason not to do things this way?

It's your call though. Let me know what you'd prefer.

Actually, there was another reason. I wanted the summary message to come after the build succeeded message. For example:

... build succeeded You can now process the pickle files in build/pickle.

I wasn't able to do this if I called this as part of Builder.finish(). If we move this here, we should also move the generation of the build succeeded message. I don't know if we want to do this.

If it was, we could end up emitting the message before any subclasses were called.
...
Actually, there was another reason. I wanted the summary message to come after the build succeeded message.

Reasonable. My idea was wrong. It should be shown on Builder.build().
Please forget this comment.

Is there any reason not to do things this way?

My worry is how to customize the epilog message on subclasses.
If they want to show more complicated message, an attribute is not extensible.
For example, it is hard to use out_suffix to the message. On the other hand, a method is easy to override in subclasses.

I can't determine the interface is important or not. So I'd like to hear your idea.

Is there any reason not to do things this way?

My worry is how to customize the epilog message on subclasses.
If they want to show more complicated message, an attribute is not extensible.
For example, it is hard to use out_suffix to the message. On the other hand, a method is easy to override in subclasses.

I can't determine the interface is important or not. So I'd like to hear your idea.

Sorry for the delay. The epilog attribute covers all the provided builders. Also, if users really need to override this, then can use properties.

@property def epilog(self): ...

Personally, I think this is adequate.

fair enough

This allows builders to emit a final epilog message containing information such as where resulting files can be found. This is only emitted if the build was successful. This allows us to remove this content from the 'make_mode' tool and the legacy 'Makefile' and 'make.bat' templates. There's room for more dramatic simplification of the former, but this will come later. Signed-off-by: Stephen Finucane <[email protected]>

These are handled by the default case. Signed-off-by: Stephen Finucane <[email protected]>

Most of these are not necessary now that we're not printing different messages for various builders. Signed-off-by: Stephen Finucane <[email protected]>

As with the Makefile previously, these are not necessary now that we're not printing anything different for various builders. Signed-off-by: Stephen Finucane <[email protected]>

It's unlikely that anyone will need to do this but at least give them the opportunity. Signed-off-by: Stephen Finucane <[email protected]>

tk0miya · 2018-01-12T12:37:27Z

sphinx/application.py

            else:
                logger.info(bold(__('build %s.') % status))
+
+            if self.statuscode == 0 and self.builder.epilog:


fair enough

tk0miya · 2018-01-12T13:13:05Z

sphinx/make_mode.py

-        if self.run_generic_build('html') > 0:
-            return 1
-        print()
-        print('Build finished. The HTML pages are in %s.' % self.builddir_join('html'))


It seems "Build finished." message has been lost. Is this intended?

Yes. We already emit build succeeded or build finished with problems here. I thought this was duplicate information.

okay, I understand

tk0miya · 2018-01-12T13:25:15Z

sphinx/templates/quickstart/Makefile_t

 PAPEROPT_a4     = -D latex_elements.papersize=a4
 PAPEROPT_letter = -D latex_elements.papersize=letter
-ALLSPHINXOPTS   = -d $(BUILDDIR)/doctrees $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) {{ rsrcdir }}
+# $(O) is meant as a shortcut for $(SPHINXOPTS)


Do you know where $(O) comes from? Is this special variable for GNU Make?

No, it's just a shortcut added when make_mode was added. @jfbu noted that he uses this so I added it here too

Is it shell variable like O=... make html ?
Okay, I almost understand. I don't know such shortcut.

Exactly. It was added as part of make mode. It's just a usability helper.

tk0miya · 2018-01-12T14:56:58Z

sphinx/templates/quickstart/Makefile_t

-	@echo "Build finished. Dummy builder generates no files."
+# Catch-all target: route all unknown targets to Sphinx
+.PHONY: Makefile
+%: Makefile


Note: this Makefile allows any argument, so this is like a Makefile for make-mode.
I think it's okay.

Yes. This will break if a user passes in an unknown builder, but Sphinx should show sufficient information. I can harden this if it would help (i.e. only allow one argument) but I don't know if that's necessary? Your call 😄

Could you show me the result of make foo bar? Is sphinx-build invoked twice?

Anyway, make-mode is not used by default. So almost of users would not see this file. So either is fine to me.

Could you show me the result of make foo bar? Is sphinx-build invoked twice?

Sure. First, using valid builder names.

$ make html dirhtml sphinx-build -b "html" -d build/doctrees source "build/html" Running Sphinx v1.7+ loading pickled environment... done building [mo]: targets for 0 po files that are out of date building [html]: targets for 0 source files that are out of date updating environment: 0 added, 0 changed, 0 removed looking for now-outdated files... none found no targets are out of date. build succeeded. The HTML pages are in build/html. sphinx-build -b "dirhtml" -d build/doctrees source "build/dirhtml" Running Sphinx v1.7+ loading pickled environment... done building [mo]: targets for 0 po files that are out of date building [dirhtml]: targets for 0 source files that are out of date updating environment: 0 added, 0 changed, 0 removed looking for now-outdated files... none found no targets are out of date. build succeeded. The HTML pages are in build/dirhtml.

Then using invalid builders.

$ make foo html sphinx-build -b "foo" -d build/doctrees source "build/foo" Running Sphinx v1.7+ Sphinx error: Builder name foo not registered or available through entry point make: *** [Makefile:96: foo] Error 2

Anyway, make-mode is not used by default. So almost of users would not see this file. So either is fine to me.

I think you mean make-mode is used by default. This was changed in Sphinx 1.6, I think? This affects the non-make-mode change. Users won't see it yet, but they will when I deprecate make mode in the future 😄

tk0miya

LGTM

tk0miya · 2018-01-12T15:32:28Z

@shimizukawa do you have any opinion? I will merge this before 1.7 feature freeze.

shimizukawa · 2018-01-13T05:56:55Z

+0.

I think that changing the old Makefile conflicts with the meaning of the --no-use-make-mode option. However, since we should remove the old Makefile feature in the near future, I think that there is no big problem with this PR.

tk0miya · 2018-01-13T09:39:24Z

Okay, let's go forward.

stephenfin · 2018-01-13T11:08:09Z

However, since we should remove the old Makefile feature in the near future, I think that there is no big problem with this PR.

To be clear, I'm hoping to do the opposite. I want to remove (well, deprecate and later remove) make mode by making sphinx-build easier to use. I won't achieve this in 1.7, but I hope to do so in 1.8.

tk0miya · 2018-01-13T11:45:32Z

I have to say no to you. We should keep make-mode for a while to keep compatibility.
There are many generated Makefile in many repository. So removing make-mode means breaking these builds.
At same time, it means non-make-mode also should not be removed for a while.

Of course, I also feel make-mode is not good. So I can understand and agree with you to remove it in future version. But is is not during 1.x. (I can't say it is okay in 2.0)

So all we can do now is refactoring it simple or creating new CLI (called sphinx and its subcommands). In any case, we have to discuss about new sphinx-build command #4320 (sorry for late, but I still remember it. I'm in busy for next release now)

stephenfin · 2018-01-13T11:51:48Z

I have to say no to you. We should keep make-mode for a while to keep compatibility. There are many generated Makefile in many repository. So removing make-mode means breaking these builds. At same time, it means non-make-mode also should not be removed for a while.

Of course, I also feel make-mode is not good. So I can understand and agree with you to remove it in future version. But is is not during 1.x. (I can't say it is okay in 2.0)

Oh, of course. I'm talking long term. We can't break users. I would expect a major version bump before we can remove it.

So all we can do now is refactoring it simple or creating new CLI (called sphinx and its subcommands). In any case, we have to discuss about new sphinx-build command #4320 (sorry for late, but I still remember it. I'm in busy for next release now)

I'm working on this. Expect a pull request by tomorrow 😄

stephenfin force-pushed the builder-success-message branch from 11f7f2e to 5f6147d Compare December 28, 2017 21:14

stephenfin commented Dec 28, 2017

View reviewed changes

jfbu reviewed Dec 28, 2017

View reviewed changes

stephenfin force-pushed the builder-success-message branch 3 times, most recently from 146cb87 to 34de6aa Compare December 29, 2017 12:57

tk0miya reviewed Dec 29, 2017

View reviewed changes

stephenfin force-pushed the builder-success-message branch from 34de6aa to 08f5976 Compare December 29, 2017 15:05

tk0miya added api builder labels Dec 30, 2017

stephenfin added 6 commits January 11, 2018 20:57

make_mode: Remove unnecessary 'make_*' functions

221dffd

These are handled by the default case. Signed-off-by: Stephen Finucane <[email protected]>

Makefile: Remove unnecessary targets

aa2c5c9

Most of these are not necessary now that we're not printing different messages for various builders. Signed-off-by: Stephen Finucane <[email protected]>

make.bat: Remove unnecessary targets

6f9a262

As with the Makefile previously, these are not necessary now that we're not printing anything different for various builders. Signed-off-by: Stephen Finucane <[email protected]>

Makefile: Make SOURCEDIR configurable

f6a045d

It's unlikely that anyone will need to do this but at least give them the opportunity. Signed-off-by: Stephen Finucane <[email protected]>

make.bat: Make SOURCEDIR configurable

b16fd2c

It's unlikely that anyone will need to do this but at least give them the opportunity. Signed-off-by: Stephen Finucane <[email protected]>

stephenfin force-pushed the builder-success-message branch from 08f5976 to b16fd2c Compare January 11, 2018 23:08

tk0miya reviewed Jan 12, 2018

View reviewed changes

tk0miya approved these changes Jan 12, 2018

View reviewed changes

tk0miya merged commit decbf20 into sphinx-doc:master Jan 13, 2018

tk0miya added a commit that referenced this pull request Jan 13, 2018

Update CHANGES for PR #4354

db361c3

stephenfin deleted the builder-success-message branch January 13, 2018 11:06

anntzer mentioned this pull request May 10, 2018

Remove unused SPHINXPROJ from new Makefile/make.bat. #4954

Merged

stephenfin mentioned this pull request Dec 21, 2019

Add unified 'sphinx' command (WIP!) #6938

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2021

Uh oh!

Conversation

stephenfin commented Dec 28, 2017

Feature or Bugfix

Purpose

Detail

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfbu Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenfin Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenfin Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tk0miya left a comment

Choose a reason for hiding this comment

Uh oh!

tk0miya commented Jan 12, 2018

Uh oh!

shimizukawa commented Jan 13, 2018

Uh oh!

tk0miya commented Jan 13, 2018

Uh oh!

stephenfin commented Jan 13, 2018

Uh oh!

tk0miya commented Jan 13, 2018

Uh oh!

stephenfin commented Jan 13, 2018

Uh oh!

Reviewers

Assignees

jfbu Dec 28, 2017 •

edited

Loading

stephenfin Dec 29, 2017 •

edited

Loading

stephenfin Jan 12, 2018 •

edited

Loading