Skip to content

[WIP] bpo-34089: Remove required (non-optional) modules from Modules/Setup.dist#8233

Closed
nascheme wants to merge 2 commits intopython:masterfrom
nascheme:empty_setup_dist
Closed

[WIP] bpo-34089: Remove required (non-optional) modules from Modules/Setup.dist#8233
nascheme wants to merge 2 commits intopython:masterfrom
nascheme:empty_setup_dist

Conversation

@nascheme
Copy link
Copy Markdown
Member

@nascheme nascheme commented Jul 10, 2018

Remove non-optional extension modules from Setup.dist. Instead, add them directly to Makefile.pre.in and Modules/config.c.in. It is a few more lines of code to do things this way but it simplifies the build. The Setup + makesetup system is still supported for people who want to use it. However, I think it is better if Setup does not contain a mixture of non-optional and optional extension modules. Instead, it should be the set of modules that the user building Python wants to explicitly include.

https://bugs.python.org/issue34089

The extensions that were previously enabled as built-in static
modules must be available in order to run setup.py.  In order to
simplify the behavior of Setup/makesetup, add them directly to the
build.
@terryjreedy terryjreedy removed their request for review July 10, 2018 23:25
@terryjreedy
Copy link
Copy Markdown
Member

As near as I can tell, patch does not affect idlelib.

@nascheme
Copy link
Copy Markdown
Member Author

Hmm, I did not request all these reviews, AFAIK. I removed a bunch of them as I think their review is not needed (although certainly welcome).

@nascheme
Copy link
Copy Markdown
Member Author

GH-8229 is trying to address some of same issues as this PR. Most people don't mess with Modules/Setup.dist or Modules/Setup. So, the complicated behaviour of the build regarding these files just causes issues without real benefits. As Victor observes, as a long time core dev, he has never modified either file. However, they cause problems for him (e.g. git bisect errors).

To address Victor's problem re "git bisect", I think we should modify the Makefile and remove some of the dependancy checking. If people are modifying Modules/Setup, they can manually force a rebuild if they need. I see the Debian Python package 'rules' file does this already (calls makesetup explicitly to ensure that everything is up-to-date).

Don't be clever and try to re-generate Makefile if Setup, makesetup
or other dependencies have changed.  When these rules were added, it
was common for each build to have its own customized Setup file.
Now, the common case is that Setup is not touched and is just a copy
of Setup.dist.  The Makefile and Module/config.c files can still be
manually re-generated using the new "makesetup" target.
@nascheme
Copy link
Copy Markdown
Member Author

I have modified the makefile to remove the dependancy checks on Setup/Setup.local/makesetup. After moving the non-optional modules to Makefile.pre.in and config.c.in, we normally should not need to re-generate the makefile. For the small number of people using a custom Setup file, I think they can use the new "makesetup" target I added if they need to regen. Otherwise, the 'configure' script generates the Makefile and that should be sufficient.

I was actually the one who originally added the dependancy checks (way back in 2001). I feel I might be entitled to remove them. :-) Back it 2001 they made more sense. People would edit Modules/Setup to enable/disable an extension module or change compile/link flags. Then, running "make" twice would be sufficient to re-build. Having to re-run 'configure' after each change was unnecessarily slow. However, these days, I don't feel bad to tell people just to run 'configure' if they have changed 'Setup'.

@Mariatta
Copy link
Copy Markdown
Member

Mariatta commented Jul 11, 2018

I'm curious why people are getting identified as CODEOWNERS, and requested review. Could be a bug with GitHub, or a bug with how we set up our own CODEOWNERS file. 🤷‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants