bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE #6489

xdegaye · 2018-04-16T22:59:48Z

https://bugs.python.org/issue32232

xdegaye · 2018-04-17T09:33:22Z

Modules/Setup.dist

-time timemodule.c	# -lm # time operations and variables
-_thread _threadmodule.c         # low-level threading interface
+posix -DPy_BUILD_CORE posixmodule.c	# posix (UNIX) system calls
+errno errnomodule.c			# posix (UNIX) errno values


Obviously errnomodule.c does not need to be compiled with -DPy_BUILD_CORE. But is it possible that other modules in this list that build correctly now with a standard linux platform without -DPy_BUILD_CORE, may need to be build with -DPy_BUILD_CORE given some specific set of macros in pyconfig.h ? Maybe it is safer then to use -DPy_BUILD_CORE for all modules currently statically built in the interpreter (and leave the commented out modules unchanged as they are built without -DPy_BUILD_CORE in setup.py).

I think we can treat those cases as bug reports as folks using custom pyconfig.h files start looking at building 3.7 based versions. (Of all the ones listed, the main one I'm surprised was able to build without -DPy_BUILD_CORE is _weakref).

The other case where I think it would make sense to reconsider omitting the setting is if this change turns out to be a performance regression.

I meant that on another platform than linux, the configure stage may define a macro in pyconfig.h that causes one of the modules where -DPy_BUILD_CORE is not used in the current PR (because the build does not fail for this module on linux) to require direct access to the interpreter internal for this platform. Maybe this is far-fetched.

Noting the resolution of this here: https://bugs.python.org/issue32232#msg315515 (Xavier checked the modules for symbols defined in pyconfig.h.in, and this concern turns out not to apply in practice)

ncoghlan

The essential approach looks good to me - just some comments inline regarding the specifics of naming and wording.

ncoghlan · 2018-04-17T10:59:14Z

Makefile.pre.in

@@ -106,7 +106,8 @@ ARFLAGS=	@ARFLAGS@
 # Extra C flags added for building the interpreter object files.
 CFLAGSFORSHARED=@CFLAGSFORSHARED@
 # C flags used for building the interpreter object files
-PY_CORE_CFLAGS=	$(PY_CFLAGS) $(PY_CFLAGS_NODIST) $(PY_CPPFLAGS) $(CFLAGSFORSHARED) -DPy_BUILD_CORE
+PY_NO_CORE_CFLAGS= $(PY_CFLAGS) $(PY_CFLAGS_NODIST) $(PY_CPPFLAGS) $(CFLAGSFORSHARED)


Bikeshed: perhaps PY_MODULE_CFLAGS or PY_STDMODULE_CFLAGS would be clearer, since these are the flags for stdlib extension modules in Modules/ that don't need direct access to the interpreter internal?

PY_STDMODULE_CFLAGS makes sense. I will make this change in the next commit.

ncoghlan · 2018-04-17T11:04:35Z

Misc/NEWS.d/next/Build/2018-04-17-00-38-19.bpo-32232.o7G_UO.rst

@@ -0,0 +1,2 @@
+Some modules configured in Modules/Setup are not anymore built with
+-DPy_BUILD_CORE.


Suggested wording:

By default, modules configured in Modules/Setup are no longer built with -DPy_BUILD_CORE. Instead, modules that specifically need that preprocessor definition include it in their individual entries.

Much better indeed :-)

ncoghlan · 2018-04-17T11:09:16Z

Modules/Setup.dist

-time timemodule.c	# -lm # time operations and variables
-_thread _threadmodule.c         # low-level threading interface
+posix -DPy_BUILD_CORE posixmodule.c	# posix (UNIX) system calls
+errno errnomodule.c			# posix (UNIX) errno values


I think we can treat those cases as bug reports as folks using custom pyconfig.h files start looking at building 3.7 based versions. (Of all the ones listed, the main one I'm surprised was able to build without -DPy_BUILD_CORE is _weakref).

The other case where I think it would make sense to reconsider omitting the setting is if this change turns out to be a performance regression.

ncoghlan · 2018-04-17T11:10:14Z

Modules/makesetup

@@ -233,7 +233,7 @@ sed -e 's/[ 	]*#.*//' -e '/^[ 	]*$/d' |
 			case $doconfig in
 			no)	cc="$cc \$(CCSHARED) \$(PY_CFLAGS) \$(PY_CPPFLAGS)";;
 			*)
-				cc="$cc \$(PY_CORE_CFLAGS)";;
+				cc="$cc \$(PY_NO_CORE_CFLAGS)";;


Assuming the name in Makefile.pre.in changes, it would also need to change here.

miss-islington · 2018-04-20T15:04:03Z

Thanks @xdegaye for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

…ILD_CORE (pythonGH-6489) Setup modules are no longer built with -DPy_BUILD_CORE by default, as using that flag may now require including additional internal-only header files. Instead, only the modules that specifically need it use that setting. (cherry picked from commit 063db62) Co-authored-by: xdegaye <xdegaye@gmail.com>

bedevere-bot · 2018-04-20T15:04:18Z

GH-6547 is a backport of this pull request to the 3.7 branch.

…ILD_CORE (GH-6489) Setup modules are no longer built with -DPy_BUILD_CORE by default, as using that flag may now require including additional internal-only header files. Instead, only the modules that specifically need it use that setting. (cherry picked from commit 063db62) Co-authored-by: xdegaye <xdegaye@gmail.com>

bpo-32232: some Setup modules are not anymore built with -DPy_BUILD_CORE

152b289

the-knights-who-say-ni added the CLA signed label Apr 16, 2018

bedevere-bot added the awaiting review label Apr 16, 2018

xdegaye commented Apr 17, 2018

View reviewed changes

ncoghlan reviewed Apr 17, 2018

View reviewed changes

Use PY_STDMODULE_CFLAGS and a better wording for the Misc/News entry

a53d6f9

xdegaye changed the title ~~bpo-32232: some Setup modules are not anymore built with -DPy_BUILD_CORE~~ bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE Apr 17, 2018

xdegaye closed this Apr 17, 2018

xdegaye reopened this Apr 17, 2018

xdegaye closed this Apr 17, 2018

xdegaye reopened this Apr 17, 2018

ncoghlan merged commit 063db62 into python:master Apr 20, 2018

bedevere-bot removed the awaiting review label Apr 20, 2018

ncoghlan added the needs backport to 3.7 label Apr 20, 2018

bedevere-bot removed the needs backport to 3.7 label Apr 20, 2018

xdegaye deleted the bpo-32232 branch April 20, 2018 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE #6489

bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE #6489

xdegaye commented Apr 16, 2018 •

edited by bedevere-bot

Loading

xdegaye Apr 17, 2018

ncoghlan Apr 17, 2018

xdegaye Apr 17, 2018

ncoghlan Apr 20, 2018

ncoghlan left a comment

ncoghlan Apr 17, 2018

xdegaye Apr 17, 2018

ncoghlan Apr 17, 2018

xdegaye Apr 17, 2018

ncoghlan Apr 17, 2018

ncoghlan Apr 17, 2018

miss-islington commented Apr 20, 2018

bedevere-bot commented Apr 20, 2018

		@@ -0,0 +1,2 @@
		Some modules configured in Modules/Setup are not anymore built with
		-DPy_BUILD_CORE.

bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE #6489

bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE #6489

Conversation

xdegaye commented Apr 16, 2018 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Apr 20, 2018

bedevere-bot commented Apr 20, 2018

xdegaye commented Apr 16, 2018 •

edited by bedevere-bot

Loading