Sphinx: Makefile options not overridable from command line

[shimizukawa]

doc/Makefile states:

#!make

# You can set these variables from the command line.

However to make the variables overridable from the command line,
the assignments must be changed:

#!patch

--- Makefile.orig       Fri Jan 31 18:02:10 2014
+++ Makefile    Fri Jan 31 18:02:33 2014
@@ -2,11 +2,11 @@
 #

 # You can set these variables from the command line.
-SPHINXOPTS   =
-SPHINXBUILD  = python ../sphinx-build.py
-SPHINXPROJ   = sphinx
-SOURCEDIR    = .
-BUILDDIR     = _build
+SPHINXOPTS  ?=
+SPHINXBUILD ?= python ../sphinx-build.py
+SPHINXPROJ  ?= sphinx
+SOURCEDIR   ?= .
+BUILDDIR    ?= _build

 # Has to be explicit, otherwise we don't get "make" without targets right.
 help:

This is also an issue in every project that adopts this Makefile.

---

• Bitbucket: https://bitbucket.org/birkenfeld/sphinx/issue/1368
• Originally reported by: minusf
• Originally created at: 2014-01-31T12:06:57.338

[shimizukawa]

_From minusf on 2014-01-31 11:24:04+00:00_

i just realised this should be fixed in sphinx.quickstart.MAKEFILE

while we are here, is the new doc/Makefile an indication of what will happen to sphinx.quickstart.MAKEFILE? I am asking because the new Makefile works only with GNU make (BSD make does not recognise % as a catchall target :( so it would be nice to keep it like it is...

[tk0miya]

Fixed by #6232.
Thank you, @svenevs!

[jfbu]

I would like to reopen discussion on this. I feel it is dangerous as commented here. It is a recent addition, so perhaps can still be partially reverted with not too much impact on Sphinx users. There are many ways it could go wrong in relation to make clean but not only. Basically I feel it is bad to obey environment variables with such a generic name as BUILDDIR.

[svenevs]

Reasonable concerns. How about SPHINXSOURCEDIR and SPHINXBUILDDIR? Or maybe it would be better / more readable to make them SPHINX_*? So SPHINX_SOURCEDIR for example.

I don't think any official releases have had the override-enabled version yet so if we act quickly it should have zero impact. Happy to do the PR if we agree on the new names.

[jfbu]

@svenevs Hi, I though too about these renames. They should not break anything because as you say the #6232 patch was merged into master for 2.1.0 which is not yet released and up to now the variables values could not be overriden from from command line or environment. However, I think some careful cautious evaluation should be made, this is why I propose at anyrate to partially revert #6232 before release. Yes, the comment in the Makefile was erroneous (thanks for the fix!), only SPHINXOPTS could be overriden from command line or from an exported environment variable. I use it often, using the short form "O=...". But allowing the others to be overriden has dangers. And sphinx-build can set directly the output directory, so people needing this can use the sphinx-build way, they don't have to use the Makefile.

[jfbu]

ah sorry I stand corrected: the SPHINXOPTS could be set from command line but an exported environment variable would not be obeyed with the former = syntax. Sorry about that.

[jfbu]

OK, on further testing with my version of Make (4.2.1), also BUILDDIR could be overridden from command line when defined as BUILDDIR = _build in the MAKEFILE.

$ make html
Sphinx v3.0.0+ en cours d'exécution
...
Les pages HTML sont dans _build/html .

versus

$ make html BUILDDIR="build"
Sphinx v3.0.0+ en cours d'exécution
....
Les pages HTML sont dans build/html .

So the effect of ?= syntax is to allow override not from command line but from environment. For reasons mentioned already this has some caveats.

[svenevs]

Continuing discussion here so it's all in one place, response to comment by @jfbu https://github.com/sphinx-doc/sphinx/pull/6232#pullrequestreview-226356390

It has always worked for me for example make html O="-D language=fr"

That PR aims to use the official version SPHINXOPTS, the $(O) trick was not documented and in terms of usage SPHINXOPTS="..." make html is clearer of intent than O="..." make html.

Perhaps what would be better is to only allow changing SPHINXOPTS and SPHINXBUILD to e.g., add a nitpicky flag, or select a different sphinx-build than would show up in the $PATH first. Changing the source / build directories seems like something we shouldn't really be concerned with supporting. The build directory _maybe_ is useful, but realistically if they are changing the source and build directories it's probably better for them to just edit the Makefile directly to suit their needs.

I don't see the necessity of a blank line at end of the file.

That was the main reason I was doing a pull request in the first place, when searching issues I found the old one about overrides not being possible and decided to fix it at the same time rather than create an issue just to fix it with a PR.

Without the extra newline, the template engine produces a Makefile without a trailing newline. All other templates have two trailing newlines, presumably for the same reason :slightly_smiling_face:

Basically, I really only want two things:

1. The ability to set SPHINXOPTS from the command line
2. I want a POSIX compliant Makefile generated, prior to that PR it was missing a trailing newline.

[svenevs]

So the effect of ?= syntax is to allow override not from command line but from environment

Correct. Example would be something like continuous integration. Typically people would want to just be able to set environment variables for different builds, but keep the same commands in the actual build script.

Edit: I probably haven't been very opaque here.

# This sets SPHINXOPTS in the environment just for this command
$ SPHINXOPTS="..." make html
# AKA it's equivalent to
$ export SPHINXOPTS="..."
$ make html
$ unset SPHINXOPTS

What you were doing is setting the actual variable for make (something that make does when processing the command line).

[jfbu]

The ability to set SPHINXOPTS from the command line

But this worked already. I mentioned O="..." because I used it

$ make html SPHINXOPTS="-D language=fr"
Sphinx v3.0.0+ en cours d'exécution

Chargement des traductions [fr]...fait

1. I want a POSIX compliant Makefile generated, prior to that PR it was missing a trailing newline.

Can you please tell me some reference? That an EOL is needed I understand but an extra blank line? I see no such requirement at GNU make docs but I am possibly wrong.

Correct. Example would be something like continuous integration. Typically people would want to just be able to set environment variables for different builds, but keep the same commands in the actual build script.

But for this there is sphinx-build or custom Makefiles as many project use.

[jfbu]

Without the extra newline, the template engine produces a Makefile without a trailing newline. All other templates have two trailing newlines, presumably for the same reason 🙂

OK, I read too quickly your comment and overlooked that you referred to a problem with template. I will try it out, thanks.

[jfbu]

yes the Makefile template needs such a blank line else as you say the produced Makefiles have no EOL, thanks a ton for noticing this :)

[svenevs]

Haha yeah I was super confused because when I looked at the code the first time I thought "hmmm, but there already is a newline here". But fixing the filtering is a lot harder than just adding the extra newline xD

What are your thoughts on this?

# You can set these variables from the command line.  For example:
#     SPHINXOPTS='-E -W -n' make html
# will run the html builder in a clean environment (-E), treating warnings
# as errors (-W), in nitpicky mode (-n).  Alternatively, you can set them as
# an environment variable:
#     export SPHINXOPTS='-E -W -n'
#     make html
SPHINXOPTS    ?=
SPHINXBUILD   ?= sphinx-build

# Overriding these two variables from the environment is disabled to avoid
# collisions with other tools.  If you seek to change these for a single call:
#     make html BUILDDIR="alternative_build_directory"
# Otherwise, simply edit this file directly to change permanently.
SOURCEDIR     = {{ rsrcdir }}
BUILDDIR      = {{ rbuilddir }}

I forgot that you could do the version you were make html SOMEVAR="value", meaning that changing the actual names of the variables is much more likely to break things. But if instead we just do _not_ do the ?= thing, we won't have to worry about collisions like you brought up.

[jfbu]

You are right that changing the variable names has some risk to break things due to already possible command line override. On the other hand I feel most people use customized Makefiles inherited from the older model shipped with Sphinx and the Makefile recipes have their own custom way to employ sphinx-build. Thus changing the format of new Makefiles is not likely to impact existing projects; but it does complicate documentation... and I much prefer indeed for time being to keep old names with = syntax.

I think the comments in your proposal are very reasonable, but recently there as a been a move to reduce the comments in conf.py to a minimum. Keeping in line with this I would suppress the comments for SOURCEDIR and BUILDDIR. And I am still hesitant about allowing environment override of SPHINXOPTS and SPHINXBUILD, knowing that build systems can and do use sphinx-build, not the shipped Makefile.

I will wait for further advice on this... (the #6271 is an important reason to be extra cautious).

[svenevs]

Sounds good. Maybe the easiest thing to do is just

1. Revert #6232.
2. Delete the comment that says "you can set these variables from the command line" entirely (people either know they can do make html SPHINXOPTS="..." or they don't and just edit it).
3. Make sure that extra newline gets in there ;)

I don't really see how #6271 relates to being able to set things via the environment / CLI, but as long as I get my newline character at the end I'm happy :)

[tk0miya]

+0 for @svenevs's comment: I don't have strong opinion for this case.

[jfbu]

I don't really see how #6271 relates to being able to set things via the environment / CLI, but as long as I get my newline character at the end I'm happy :)

@svenevs with BUILDDIR ?= ... and if BUILDDIR is accidentally set (and exported) in environment to some location having nothing to do with the Sphinx project, a make clean can and will have catastrophic consequences. For example, user does make html but the files are not found in the BUILDDIR from the Makefile; then without much ado, the user does make clean. Then half of the hard disk gets wiped out.

We can not distributed such risky Makefiles, hence at #6303 I revert the ?=. If the user wants it, it can be added by hand.

[svenevs]

Yes I agree, sorry I got my wires crossed and thought you were saying there would be problems even if we removed ?= from build/source directory.

[svenevs]

With #6303 merged we can close this again :interrobang:

[jfbu]

@svenevs closing, thanks for your contribution!