-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-32430: Rename Modules/Setup.dist to Modules/Setup #8229
Conversation
Remove the necessity to copy the former manually to the latter when updating the local source tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I hate the Modules/Setup.dist warning/error: it makes "git bisect" much harder, and I get the warning often when I change the revision in Git using git checkout :-( I never ever modified Modules/Setup nor Modules/Setup.dist.
I think this might be broken if you are building in a separate tree. Setup.dist and Setup are possibly in different dirs. Going to test that. |
Yeah, it breaks if you build in a separate directory. Fixing that seems not so easy. It will also break the Debian Python build as it is expecting Setup.dist to be there. Perhaps GH-8233 is a better approach. I.e. instead of renaming, prune Setup.dist down to something that doesn't contain any extensions by default. I added some comments to that PR to address Victor's problem. |
What would it entail to fix it? |
A key thing to realize is that "$srcdir/Modules/" might not be the same as "Modules/". The latter path refers to the build dir and the Setup file would not exist there unless configure copies it (like the current behavior for Setup) or someone manually creates it (as I suspect some 3rd party build systems might do). So, if you want to avoid the file copy on configure, I guess you have to check both places for a Setup file. makesetup could do that internally but it is passed the setup file as an argument. Making everything work seems a bit tricky but I can try to make a patch if we want to pursue this approach. Maybe a simpler change would achieve your goal. Make 'configure' unconditionally create Modules/Setup from $srcdir/Modules/Setup.dist. My guess is that no one would actually be hurt by that. You would be if you run configure, modify Modules/Setup, run configure again and expect that your Setup changes are preserved. After you make the copy unconditional, you can remove from the makefile the check for a newer Modules/Setup.dist. I think that change solves the various issues we are seeing. @vstinner Does your bisect process run "configure" before each build? What is the error you are seeing with the current Setup/Setup.dist behavior? |
Can't we instead simply read the |
Relying on $srcdir/Modules/Setup LGTM. |
If the user creates as Modules/Setup file in the build directory, it would possibly be ignored (separate source and build dirs). Looking at Debian's build, I think it does that. Also, Modules/getpath.c uses the presence of Modules/Setup to detect if 'python' is running in the build directory. I think that is trivially fixed by using Modules/Setup.local instead. Or, maybe looking for Modules/config.c makes more sense. In bpo-32430, I attached a patch that does the following:
That's better in the case that people are expecting to still read Modules/Setup.dist. They can still manually copy Setup.dist to Setup and then make their own modifications. It is bad in if they expect to see Modules/Setup. An alternative idea would be to change 'configure' to unconditionally copy Modules/Setup.dist to Modules/Setup. That idea has a problem in that people might expect their local changes to Setup to be preserved on re-running of 'configure'. That would seem like a rare case though. |
Indeed. But I don't think that's a problem. The user will probably notice and will have to adjust their practice. |
I've fixed the PR to work with out-of-tree builds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I tested manually to build (and install) Python outside its source tree, and it works as expected.
@Yhg1s Any comment about this PR? |
There are a few potential problems, whether they are show-stoppers, I don't know.
I guess both of these points apply only if you are using separate build directories. I gather that Antoine and Victor don't use them. In fact, I suspect the majority of Python core developers don't use them because over the years the build has gotten broken in that respect and it seems no one notices for a while. Supporting a separate build directory adds a large amount of complexity to the build. So, even though it is my preferred way to build Python, I'm sympathetic to the idea of throwing it out as complication we don't need. However, we either should support it properly or not at all. Another thing is to consider who the end user of the build system will be. Obviously core Python devs use it all the time. However, I expect at this time, the average Python user does not build themselves. So, aside from core devs, most of the end users of the build system are 3rd party distributors like Red Hat, Google, Enthought, etc. I would expect many of these builders do use a customized Setup file. So, aside from core developers, we could be impacting a lot of the people using the Python build system. We should not change how the build works in this respect without getting their feedback, IMHO. |
I think people who want to customize
I might be wrong, but I don't think it's a big deal.
It seems to me that making the build simpler is a good thing in itself.
Ok, let's CC @doko42 here. Do you have other people in mind? |
Matthias Klose will likely be impacted (Debian Python packager). I don't know the Red Hat people. Marc-André Lemburg perhaps. Companies like Facebook, Google, Dropbox probably have their own internal builds. An idea occurs to me now. We could apply this PR as it is but also add a new configure option. I know you don't like to complicate the build scripts but the option is simple. E.g. --with-modules-setup=. If provided, the makesetup call in 'configure' would use that file. Then, builders that don't want to use the Setup from the source tree can provide their own. That seems clean and straight-forward to me. You will avoid having to modify $srcdir/Modues/Setup, which is undesirable, IMHO. I'll try to make a patch. |
IMHO, the only serious potential problem is the compatibility breakage of build systems. Adding an option to undo it would not solve that issue. |
When you have breakage, they have to fix it. So, how do they fix it? If we break the old way of the doing things, we need to provide a new (hopefully better) way. Requiring them to modify Setup in the source tree is not a good way, IMHO. Maybe a better fix would be to call 'makesetup' directly, passing it their own Setup file. I don't find that very elegant though. |
Again: packagers routinely patch files in the source tree. It's not a new thing. See for example the Debian patches: https://sources.debian.org/src/python3.7/3.7.0-1/debian/patches/ |
Okay, patching the file seems like a decent solution. If they don't like that, they can still call 'makesetup' themselves. I'm still going to make a patch that adds a configure option. It is quite simple but we can evaluate that change separately from this PR. This one LGTM and I think it can be merged. If people like @doko42, Thomas Wouters have concerns, we can address them after merging. |
I've added a What's New entry and am going to merge soon. |
Remove the necessity to copy the former manually to the latter when updating the local source tree.
https://bugs.python.org/issue32430