Skip to content

Conversation

@xdegaye
Copy link
Contributor

@xdegaye xdegaye commented Nov 3, 2017

Copy link
Member

@vstinner vstinner left a comment

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.

Makefile.pre.in Outdated

# Default target
all: @DEF_MAKE_ALL_RULE@
all: check-clean-src @DEF_MAKE_ALL_RULE@
Copy link
Member

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?

Copy link
Contributor Author

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 \
Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Member

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 \
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. 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.

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

Copy link
Member

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.

Makefile.pre.in Outdated
# 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" ; \
Copy link
Member

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 ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Copy link
Member

@vstinner vstinner left a comment

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 \
Copy link
Member

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.

@brettcannon brettcannon removed their request for review November 7, 2017 17:43
@xdegaye xdegaye merged commit 0de9285 into python:master Nov 8, 2017
@miss-islington
Copy link
Contributor

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

@xdegaye xdegaye deleted the bpo-31934 branch November 8, 2017 15:04
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-4340 is a backport of this pull request to the 3.6 branch.

@bedevere-bot
Copy link

GH-4342 is a backport of this pull request to the 2.7 branch.

xdegaye added a commit that referenced this pull request Nov 8, 2017
xdegaye added a commit that referenced this pull request Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants