bpo-31934: Abort when building out of a not clean source tree by xdegaye · Pull Request #4255

bpo-31934: Abort when building out of a not clean source tree by xdegaye · Pull Request #4255 · python/cpython

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I was bitten multiple times by this annoying bug. It may useful to backport this helper to Python 2.7 and 3.6.


		# Default target
		all: @DEF_MAKE_ALL_RULE@
		all: check-clean-src @DEF_MAKE_ALL_RULE@

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add this dependency to $(BUILD_PYTHON) instead, to fix more "make" commands?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is much better and would cover all cases I think.

BTW a '.PHONY' declaration is missing for check-clean-src.


		# Check that the source is clean when building out of source.
		check-clean-src:
		@if test -n "$(VPATH)" -a -f "$(srcdir)/Programs/python.o"; then \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use Objects/object.o instead, since the compilation may have succeed to build object.o but failed to build python.o (or failed before trying to build python.o).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually Programs/python.o is the first object built by the Makefile as it is the first prerequisite in the $(BUILDPYTHON) rule.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually Programs/python.o is the first object built by the Makefile as it is the first prerequisite in the $(BUILDPYTHON) rule.

Oh ok, I see. I that case, it's fine :-)

		# Default target
		all: @DEF_MAKE_ALL_RULE@
		build_all: $(BUILDPYTHON) oldsharedmods sharedmods gdbhooks Programs/_testembed python-config
		build_all: check-clean-src $(BUILDPYTHON) oldsharedmods sharedmods gdbhooks \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to put check-clean-src in $(BUILDPYTHON). I tested, it works:

# Build the interpreter
$(BUILDPYTHON):	check-clean-src Programs/python.o $(LIBRARY) $(LDLIBRARY) $(PY3LIBRARY)
	$(LINKCC) $(PY_LDFLAGS) $(LINKFORSHARED) -o $@ Programs/python.o $(BLDLIBRARY) $(LIBS) $(MODLIBS) $(SYSLIBS) $(LDLAST)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem is that in this case the $(BUILDPYTHON) target is rebuilt even when it is up to date, for the following reason:

A phony target should not be a prerequisite of a real target file;
if it is, its recipe will be run every time make goes to update that file.

This is a quote from Phony Targets.

Another minor problem is that the recipe of check-clean-src may be run multiple times.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oh, I didn't know.

		# Check that the source is clean when building out of source.
		check-clean-src:
		@if test -n "$(VPATH)" -a -f "$(srcdir)/Programs/python.o"; then \
		echo "Error: The source directory is not clean" ; \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is not easy to understand. I propose:

		echo "Error: The source directory ($(srcdir)) is not clean" ; \
		echo "Building Python out of the source tree ($(abs_builddir)) requires a clean source tree ($(abs_srcdir)" ; \
		echo "Try to run: make -C \"$(srcdir)\" clean" ; \

At least, the proposed command worked for me ;-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

		# Default target
		all: @DEF_MAKE_ALL_RULE@
		build_all: $(BUILDPYTHON) oldsharedmods sharedmods gdbhooks Programs/_testembed python-config
		build_all: check-clean-src $(BUILDPYTHON) oldsharedmods sharedmods gdbhooks \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oh, I didn't know.

Thanks @xdegaye for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖

Sorry, @xdegaye, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0de92859caf25e65fc968d4bb68626e9ba21b851 3.6

Sorry, @xdegaye, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0de92859caf25e65fc968d4bb68626e9ba21b851 2.7

xdegaye added a commit that referenced this pull request

Nov 8, 2017

xdegaye added a commit that referenced this pull request

Nov 8, 2017