classification
Title: Simplify Modules/Setup{,.dist,.local}
Type: enhancement Stage: resolved
Components: Build Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, eric.smith, koobs, martin.panter, mdk, nascheme, pitrou, twouters, vstinner, xdegaye, yan12125
Priority: normal Keywords: patch

Created on 2017-12-27 11:15 by mdk, last changed 2018-09-06 07:17 by mdk. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5062 open xdegaye, 2017-12-31 10:28
PR 8229 merged pitrou, 2018-07-10 19:35
PR 8260 closed nascheme, 2018-07-12 00:53
PR 5015 mdk, 2018-09-06 07:17
Messages (26)
msg309079 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2017-12-27 11:15
Related to: https://bugs.python.org/issue32429 

The bahavior of Modules/Setup{,.dist,.local} looks inherited from an old time:

- A Setup.dist exist
- ./configure copies it to Setup (cp $srcdir/Modules/Setup.dist Modules/Setup)
- ./configure installs a dummy Modules/Setup.local file
- One can add an optional things to Setup.local
- One can have to modify Setup itself (to modify things)

From this point in time, Setup is newer than Setup.dist (as copied from it), the following can happen:

- The Setup file is manually modified, it's still newer than Setup.dist, it does not change the situation
- The Setup.dist file gets updated (via a git checkout or git merge), the Makefile detects it and warn the user:
  - Either the user modified the Setup file and don't want it to be overridden
  - Either the user didn't modified it and just want the updated version

We don't know, so the Makefile is printing a warning telling he don't know and we have to fix the situation. First problem is: this warning is invisible due to the high load of Makefile prints, this is the issue https://bugs.python.org/issue32429.

Proposed solutions are:

- Xavier de Gaye: Use Setup.dist instead of Setup when Setup is missing (https://bugs.python.org/issue32429#msg309078)
- Just keep a single versionned Setup file, let those who modify it use branches to commit their modifications

Marc-Andre Lemburg noted that for those using a sources tarballs, in the single Setup file case, it's not nice for them to loose the original content of Setup when modifying it, and being able to do a `cp Modules/Setup.dist Modules/Setup` to restore it is nice.

Personally I'll go for a single Setup file (No .dist, no .local, no copy), that one can modify and commit, delegating the pain of merging to git, as I prefer simple solutions.

I don't think the importance of keeping a ".dist" backup is worth the pain and we're not doing it for other files (If one using a source tarball is modifying a .c file, the original content is lost too, and we'll not provide .c.dist files).
msg309081 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-27 11:54
> Personally I'll go for a single Setup file (No .dist, no .local, no copy), that one can modify and commit, delegating the pain of merging to git, as I prefer simple solutions.

+1 for this.
msg309273 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-31 10:31
The Setup files are configuration files, so their handling is different from the handling of Makefile.pre.in or the handling of the source files.

The only reference to Setup.local in the Python documentation is at https://docs.python.org/3/extending/extending.html#compilation-and-linkage, so most users that need to configure the build with a Setup file are probably using Modules/Setup instead of Modules/Setup.local and the solution that implements this enhancement cannot overwrite an existing Modules/Setup file. This solution must also be robust and transparent when switching to/from a branch where the solution is not implemented yet (a bpo branch created earlier).

PR 5062 adds the ``--enable-use-setup-dist`` configure option to use Modules/Setup.dist instead of Modules/Setup to build the Makefile. The build behavior is unchanged when this option is not used.
msg309281 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-31 12:55
I would hope to simplify the build process, not to complicate it.  In other words, I think the PR is a step in the wrong direction, *except* if using Setup.dist becomes the default and the other way is clearly marked deprecated.
msg309282 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-31 13:03
Note the use case we're trying to preserve here is probably extremely rare.  I've never needed it myself, and I've never seen it done by anyone else.  We're talking about a very small demographics who's probably skilled enough to handle a little complexity in their workflow.
msg309283 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-31 14:01
> and I've never seen it done by anyone else

In msg294174 Thomas says he intends to use it for Python at Google in order to "avoid third-party libraries even when they are available on the build system".

I do not have a strong opinion about this. Implementing the reverse configure option '--enable-custom-setup' is straightforward (actually this is what I did upon the first shot at this PR), Setup.dist becomes the default then and Thomas can still use Setup.local to add the *disabled* modules. It is annoying that there is no documentation on the build process that can be modified to describe the deprecation in details. Do you think a What'sNew entry is sufficient ?
msg309285 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-31 14:06
First, I would count Google in the extremely skilled category :-)
(cc'ing Thomas so that he can chime in)

Second, if someone are doing a custom build of Python, they are very likely modifying something else than Modules/Setup (*).  So they already have some kind of custom branch or fork of CPython tracking their own customizations.  Why Modules/Setup can't be handled the same way I'm not sure I understand.

(*) see for example the Anaconda build of Python, which modifies several files but *not* Modules/Setup: https://github.com/AnacondaRecipes/python-feedstock/
msg309286 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-31 14:06
> It is annoying that there is no documentation on the build process that can be modified to describe the deprecation in details. Do you think a What'sNew entry is sufficient ?

Yes, I think so.  Also my hope is to remove it in 3.8 :-)
msg309287 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-31 14:36
Yes 3.8 and this leaves plenty of time for writing some documentation on the build process :-)
msg309613 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2018-01-07 11:27
I also think PR 5062 complicate the thing I'm trying to simplify, it's the wrong direction.

I also don't understand why having a single versioned `Setup` file is not enough, can someone give a clear explanation on this point?
msg309654 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2018-01-08 01:00
FWIW there was documentation in the README about the Setup files, removed in Subversion r57681 (= Git revision 1c896e3). It looks like it still survives in the 2.7 version.
msg309698 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2018-01-09 13:49
We do use Setup/Setup.local at Google (and have for many years now), and I find it very useful. (FWIW, the 'we would use' comment in msg294174 was about the new '*disabled*' feature, not about Setup files in general.) Avoiding local changes is also important to us, because it makes it much easier to track upstream changes. If anyone was thinking of getting rid of Modules/Setup, please don't, at least not without something more sensible than setup.py to replace it :)

That said, I don't really care about the Setup.dist/Setup distinction. I vaguely recall the situation before we had Setup.dist, which I guess was slightly worse, but primarily for people who build from the source repo. Having a Setup.local file is convenient as long as the Setup file is necessary for the base build, but Setup/Setup.dist doesn't matter.
msg310312 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-01-20 01:07
I like the idea of removing the Modules/Setup.dist => Modules/Setup copy which removes immediately the annoying warning (bpo-32429).

> ... that one can modify and commit

I'm not sure about the "commit" part. I see Modules/Setup as a configuration file. Python provides a default configuration, fine.

Why not adding an option in Makefile or configure to allow to specify a different path to a custom Setup file?

./configure --setup-file=google_uber_cool_setup_file && make
msg310343 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-01-20 13:17
> Why not adding an option in Makefile or configure to allow to specify a different path to a custom Setup file?

Because we don't want to maintain more config options?  Really, I don't to want to further complicate our build process.
msg310345 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-01-20 13:22
> Because we don't want to maintain more config options?  Really, I don't to want to further complicate our build process.

I don't see how adding an option to make one path configurable would make the build process more complicated.

It seems like Xavier and Thomas do use Modules/Setup for their need, and for me it's strange to have to modify Modules/Setup (or Modules/Setup.dist) if it's tracked by Git. If I modify it, IMHO it's a configuration file, and it shouldn't be part of Python code base. It's fine to have a default configuration, but if you don't want to  use the default, just pass the path to your own config. No?
msg310346 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-01-20 13:25
Le 20/01/2018 à 14:22, STINNER Victor a écrit :
> 
> I don't see how adding an option to make one path configurable would make the build process more complicated.

Do you think implementing and maintaing an option has no cost?

> It seems like Xavier and Thomas do use Modules/Setup for their need, and for me it's strange to have to modify Modules/Setup (or Modules/Setup.dist) if it's tracked by Git.

Distributors routinely modify files that are tracked by git.
Modules/Setup doesn't have to be an exception.

See Anaconda example I posted above.  Also you can look up packaging by
RedHat or any other distro.
msg310348 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2018-01-20 14:28
I don't see a use case where one want to edit a file without having his work being tracked by git, can someone shed a light to me on this?

I understand that to modify a single configuration point (a path, whatever) a configure option is the right way and is not versioned by git, but Modules/Setup is a file, not a flag.

But, I assumed the whole time Modules/Setup modifications are not trivial, but the result of some work worth versioning, unlike a single configure option, am I wrong?
msg310372 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2018-01-21 12:56
I fully concur with all points in Victor msg310345.
msg310373 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2018-01-21 13:49
A solution to this issue must deal with the following use case:

A developer has made changes to Modules/Setup and is tracking these changes in a VCS, for example with mercurial or with a git submodule through a symlink. By the time he is about to pull the commit that is fixing this issue from the Python repository, he is also experimenting with a new modification in Modules/Setup that he has not commited or staged yet in his own mercurial or git submodule repository or whatever.

IMO a solution to this issue that overwrites the current user changes when he is merging the corresponding commit is not acceptable.
msg311752 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2018-02-06 23:00
Taking arguments into account, this solution may work:

- Continue to distribute a Setup.dist
- Add a configure option to change it, as Victor said --setup-file should do it, the default value should 
be Setup.dist
- Remove the `cp Setup.dist Setup`

This solution fixes the issue32429 as there is no longer a copy, it does not starts to track the Setup file so it should nor introduce any issue while switching to and from branches with this modification, and allow one to have multiple named configurations.
msg321395 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-07-10 20:00
I've created a PR that renames Setup.dist to Setup and removes extraneous logic.
msg321493 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2018-07-11 18:12
PR 8229 doesn't work if the build dir is separate from the src dir (mentioned in the PR comments).  The attached patch (makesetup_fallback_dist.txt) changes makesetup to fallback to using $srcdir/Modules/Setup.dist if Modules/Setup doesn't exist.  I removed the copying of Setup.dist to Setup.  Also, I removed the dependencies in the makefile that look for an out-of-date Setup file and the ones that automatically run makesetup.  There is now a "makesetup" target with no deps.  You would have to run "make makesetup" if you want to regenerate config.c and the Makefile.  Otherwise, running "configure" would also do the job.

One possible issue with the patch: I expect that some build systems expect that, after configure, Modules/Setup will exist in the build dir and can be modified.  For that reason, I think a better fix is to make 'configure' unconditionally create Modules/Setup from $srcdir/Modules/Setup.dist.

I'm attaching as a patch as I'm not sure about creating a bunch of half-baked PRs to show ideas.  It seems to me that a small patch as a text attachment is better.  Maybe I'm a dinosaur. ;-P
msg321570 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2018-07-12 16:45
I removed the "makesetup_fallback_dist.txt".  PR-8260 is a more polished implementation of the same idea.
msg321744 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-07-16 17:03
New changeset 961d54c5c1916c09883ebcf7191babc969e5a5cf by Antoine Pitrou in branch 'master':
bpo-32430: Rename Modules/Setup.dist to Modules/Setup (GH-8229)
https://github.com/python/cpython/commit/961d54c5c1916c09883ebcf7191babc969e5a5cf
msg321745 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-07-16 17:04
With PR #8229 pushed the distinction between Setup.dist and Setup has disappeared: you have a Modules/Setup file in the source tree and that's where you do any changes, which you are responsible to track yourself (e.g. using a patch file or a git fork).
msg321768 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-16 20:33
Thank you Antoine for fixing this issue! This issue annoyed me forever, but
I never tried to fix it. It was very annoying when using git bisect for
example.
History
Date User Action Args
2018-09-06 07:17:13mdksetpull_requests: + pull_request8537
2018-07-16 20:33:25vstinnersetmessages: + msg321768
2018-07-16 17:04:41pitrousetstatus: open -> closed
resolution: fixed
messages: + msg321745

stage: patch review -> resolved
2018-07-16 17:03:13pitrousetmessages: + msg321744
2018-07-13 23:48:04eric.smithsetnosy: + eric.smith
2018-07-12 16:45:41naschemesetmessages: + msg321570
2018-07-12 16:44:05naschemesetfiles: - makesetup_fallback_dist.txt
2018-07-12 03:00:07yan12125setnosy: + yan12125
2018-07-12 00:53:34naschemesetpull_requests: + pull_request7793
2018-07-11 18:13:42naschemesetfiles: + makesetup_fallback_dist.txt
2018-07-11 18:12:59naschemesetnosy: + nascheme, - yan12125
messages: + msg321493
2018-07-11 17:41:12yan12125setnosy: + yan12125
2018-07-10 20:00:47pitrousetmessages: + msg321395
2018-07-10 19:41:32pitrousetversions: + Python 3.8, - Python 3.7
2018-07-10 19:35:35pitrousetpull_requests: + pull_request7767
2018-03-02 12:36:16koobssetnosy: + koobs
2018-02-06 23:00:29mdksetmessages: + msg311752
2018-01-21 13:49:52xdegayesetmessages: + msg310373
2018-01-21 12:56:29xdegayesetmessages: + msg310372
2018-01-20 14:28:25mdksetmessages: + msg310348
2018-01-20 13:25:19pitrousetmessages: + msg310346
2018-01-20 13:22:44vstinnersetmessages: + msg310345
2018-01-20 13:17:50pitrousetmessages: + msg310343
2018-01-20 01:07:18vstinnersetnosy: + vstinner
messages: + msg310312
2018-01-09 13:49:45twouterssetmessages: + msg309698
2018-01-08 01:00:22martin.pantersetnosy: + martin.panter
messages: + msg309654
2018-01-07 11:27:19mdksetmessages: + msg309613
2017-12-31 14:36:44xdegayesetmessages: + msg309287
2017-12-31 14:06:51pitrousetmessages: + msg309286
2017-12-31 14:06:21pitrousetnosy: + twouters
messages: + msg309285
2017-12-31 14:01:13xdegayesetmessages: + msg309283
2017-12-31 13:03:19pitrousetmessages: + msg309282
2017-12-31 12:55:46pitrousetmessages: + msg309281
2017-12-31 10:31:29xdegayesetnosy: + xdegaye
messages: + msg309273
2017-12-31 10:28:10xdegayesetkeywords: + patch
stage: patch review
pull_requests: + pull_request4938
2017-12-27 11:54:28pitrousetversions: - Python 2.7, Python 3.6
nosy: + barry, pitrou

messages: + msg309081

type: enhancement
2017-12-27 11:15:44mdksetcomponents: + Build
versions: + Python 2.7, Python 3.6, Python 3.7
2017-12-27 11:15:03mdkcreate