msg309079 - (view) |
Author: Julien Palard (mdk) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2018-01-21 12:56 |
I fully concur with all points in Victor msg310345.
|
msg310373 - (view) |
Author: Xavier de Gaye (xdegaye) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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.
|
msg341533 - (view) |
Author: anthony shaw (anthony shaw) |
Date: 2019-05-06 15:34 |
PR GH-5062 is still open in GitHub, is this PR still required?
|
msg341540 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2019-05-06 15:37 |
> PR GH-5062 is still open in GitHub, is this PR still required?
It's not. I closed it, thanks.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:56 | admin | set | github: 76611 |
2019-05-06 15:37:43 | vstinner | set | messages:
+ msg341540 |
2019-05-06 15:34:14 | anthony shaw | set | nosy:
+ anthony shaw messages:
+ msg341533
|
2018-09-06 07:17:13 | mdk | set | pull_requests:
+ pull_request8537 |
2018-07-16 20:33:25 | vstinner | set | messages:
+ msg321768 |
2018-07-16 17:04:41 | pitrou | set | status: open -> closed resolution: fixed messages:
+ msg321745
stage: patch review -> resolved |
2018-07-16 17:03:13 | pitrou | set | messages:
+ msg321744 |
2018-07-13 23:48:04 | eric.smith | set | nosy:
+ eric.smith
|
2018-07-12 16:45:41 | nascheme | set | messages:
+ msg321570 |
2018-07-12 16:44:05 | nascheme | set | files:
- makesetup_fallback_dist.txt |
2018-07-12 03:00:07 | yan12125 | set | nosy:
+ yan12125
|
2018-07-12 00:53:34 | nascheme | set | pull_requests:
+ pull_request7793 |
2018-07-11 18:13:42 | nascheme | set | files:
+ makesetup_fallback_dist.txt |
2018-07-11 18:12:59 | nascheme | set | nosy:
+ nascheme, - yan12125 messages:
+ msg321493 |
2018-07-11 17:41:12 | yan12125 | set | nosy:
+ yan12125
|
2018-07-10 20:00:47 | pitrou | set | messages:
+ msg321395 |
2018-07-10 19:41:32 | pitrou | set | versions:
+ Python 3.8, - Python 3.7 |
2018-07-10 19:35:35 | pitrou | set | pull_requests:
+ pull_request7767 |
2018-03-02 12:36:16 | koobs | set | nosy:
+ koobs
|
2018-02-06 23:00:29 | mdk | set | messages:
+ msg311752 |
2018-01-21 13:49:52 | xdegaye | set | messages:
+ msg310373 |
2018-01-21 12:56:29 | xdegaye | set | messages:
+ msg310372 |
2018-01-20 14:28:25 | mdk | set | messages:
+ msg310348 |
2018-01-20 13:25:19 | pitrou | set | messages:
+ msg310346 |
2018-01-20 13:22:44 | vstinner | set | messages:
+ msg310345 |
2018-01-20 13:17:50 | pitrou | set | messages:
+ msg310343 |
2018-01-20 01:07:18 | vstinner | set | nosy:
+ vstinner messages:
+ msg310312
|
2018-01-09 13:49:45 | twouters | set | messages:
+ msg309698 |
2018-01-08 01:00:22 | martin.panter | set | nosy:
+ martin.panter messages:
+ msg309654
|
2018-01-07 11:27:19 | mdk | set | messages:
+ msg309613 |
2017-12-31 14:36:44 | xdegaye | set | messages:
+ msg309287 |
2017-12-31 14:06:51 | pitrou | set | messages:
+ msg309286 |
2017-12-31 14:06:21 | pitrou | set | nosy:
+ twouters messages:
+ msg309285
|
2017-12-31 14:01:13 | xdegaye | set | messages:
+ msg309283 |
2017-12-31 13:03:19 | pitrou | set | messages:
+ msg309282 |
2017-12-31 12:55:46 | pitrou | set | messages:
+ msg309281 |
2017-12-31 10:31:29 | xdegaye | set | nosy:
+ xdegaye messages:
+ msg309273
|
2017-12-31 10:28:10 | xdegaye | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request4938 |
2017-12-27 11:54:28 | pitrou | set | versions:
- Python 2.7, Python 3.6 nosy:
+ barry, pitrou
messages:
+ msg309081
type: enhancement |
2017-12-27 11:15:44 | mdk | set | components:
+ Build versions:
+ Python 2.7, Python 3.6, Python 3.7 |
2017-12-27 11:15:03 | mdk | create | |