Skip to content
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

Merged
merged 3 commits into from
Jul 16, 2018

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jul 10, 2018

Remove the necessity to copy the former manually to the latter when updating the local source tree.

https://bugs.python.org/issue32430

Remove the necessity to copy the former manually to the latter when updating the local source tree.
@pitrou
Copy link
Member Author

pitrou commented Jul 10, 2018

@nascheme

Copy link
Member

@vstinner vstinner left a 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.

@nascheme
Copy link
Member

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.

@nascheme
Copy link
Member

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.

@pitrou
Copy link
Member Author

pitrou commented Jul 11, 2018

Yeah, it breaks if you build in a separate directory. Fixing that seems not so easy.

What would it entail to fix it?

@nascheme
Copy link
Member

nascheme commented Jul 11, 2018

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?

@pitrou
Copy link
Member Author

pitrou commented Jul 11, 2018

Can't we instead simply read the Setup file directly from $srcdir/Modules? I don't see any reason for that file to be any more special than other files.

@vstinner
Copy link
Member

Relying on $srcdir/Modules/Setup LGTM.

@nascheme
Copy link
Member

Can't we instead simply read the Setup file directly from $srcdir/Modules? I don't see any reason for that file to be any more special than other files.

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:

  • stop copying Modules/Setup.dist to Setup
  • remove from the makefile the check for an out-of-date Setup file
  • remove from the makefile the automatic regeneration of Makefile and Modules/config.c
  • change makesetup to fallback to Modules/Setup.dist if Modules/Setup doesn't exist

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.

@pitrou
Copy link
Member Author

pitrou commented Jul 12, 2018

If the user creates as Modules/Setup file in the build directory, it would possibly be ignored

Indeed. But I don't think that's a problem. The user will probably notice and will have to adjust their practice.

@pitrou
Copy link
Member Author

pitrou commented Jul 12, 2018

I've fixed the PR to work with out-of-tree builds.

Copy link
Member

@vstinner vstinner left a 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.

@pitrou
Copy link
Member Author

pitrou commented Jul 12, 2018

@Yhg1s Any comment about this PR?

@nascheme
Copy link
Member

nascheme commented Jul 12, 2018

There are a few potential problems, whether they are show-stoppers, I don't know.

  1. If you want to customize Setup, you have to modify the Setup file in the source directory, not in the build directory. That's counter to the main idea of separate source/build directories, IMHO. Ideally (broken at this point for Python, I think), the source directory should be able to be read-only. All build files would be in the build directory. By removing the build directory, you should be to back to a clean source tree. Also, you should be able to have multiple builds, with different configurations, all using the same source tree. Because Setup is being taken from the source dir, that doesn't work after this change, assuming you want to customize Setup.

  2. As mentioned previously, if you create Modules/Setup in the build directory, it will be ignored. 3rd party build systems that do this will have to be fixed. That's not a big deal but it seems like breakage for no strong reason. What are we gaining vs bpo-32430: During 'configure', don't copy Setup.dist to Setup. #8260? I guess makesetup is simpler in this PR. Is there another benefit?

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.

@pitrou
Copy link
Member Author

pitrou commented Jul 12, 2018

If you want to customize Setup, you have to modify the Setup file in the source directory, not in the build directory.

I think people who want to customize Setup probably customize other things, at least setup.py. Packagers routinely apply many patches to the Python source tree; patching Setup can be part of that routine.

if you create Modules/Setup in the build directory, it will be ignored. 3rd party build systems that do this will have to be fixed.

I might be wrong, but I don't think it's a big deal.

What are we gaining vs GH-8260? I guess makesetup is simpler in this PR. Is there another benefit?

It seems to me that making the build simpler is a good thing in itself.

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.

Ok, let's CC @doko42 here. Do you have other people in mind?

@nascheme
Copy link
Member

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.

@pitrou
Copy link
Member Author

pitrou commented Jul 12, 2018

IMHO, the only serious potential problem is the compatibility breakage of build systems. Adding an option to undo it would not solve that issue.

@nascheme
Copy link
Member

nascheme commented Jul 12, 2018

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.

@pitrou
Copy link
Member Author

pitrou commented Jul 12, 2018

Requiring them to modify Setup in the source tree is not a good way, IMHO.

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/
The conda-forge patches: https://github.com/conda-forge/python-feedstock/tree/master/recipe

@nascheme
Copy link
Member

Again: packagers routinely patch files in the source tree.

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.

@pitrou
Copy link
Member Author

pitrou commented Jul 16, 2018

I've added a What's New entry and am going to merge soon.

@pitrou pitrou merged commit 961d54c into python:master Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants