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

setup.py setup() not detected in __name__ == '__main__' block #1611

Closed
rogerbinns opened this issue Sep 13, 2023 · 6 comments · Fixed by #1613
Closed

setup.py setup() not detected in __name__ == '__main__' block #1611

rogerbinns opened this issue Sep 13, 2023 · 6 comments · Fixed by #1613

Comments

@rogerbinns
Copy link

Description

My setup.py setup() includes:

python_requires=">=3.8"

However cibuildwheel still tries and fails to compile under Python 3.6.

I understand there is CIBW_BUILD / CIBW_SKIP but that is then duplicating the python requires information.

I can add a [project] section to pyproject.toml but that leads to a lot of problems because it ends up fighting with setup() parameters and they really don't like it.

I believe cibuildwheel should establish the Python version support automatically whether it comes from setuptools or pyproject.toml, and not try to build on unsupported versions. My pyproject.toml is:

[build-system]
requires = ["setuptools"]
build-backend = "setuptools.build_meta"

Build log

https://github.com/rogerbinns/apsw/actions/runs/6175182758/job/16761477543

CI config

https://github.com/rogerbinns/apsw/actions/runs/6175182758/workflow

@henryiii
Copy link
Contributor

henryiii commented Sep 13, 2023

This requires parsing the AST and trying to figure out this setting. Cibuildwheel actually does exactly this (though some people dislike it). You have a non-standard if __name__ == "__main__" wrapping your setup call, which I assume is what is confusing cibuildwheel's static investigation of your setup.py.

What I'd highly recommend is moving as many of these keyword arguments as possible out of your setup.py's setup() call and putting them in setup.cfg. (Or pyproject.toml, but it sounded like you didn't want to do that, that makes a bigger differences in defaults and it also requires setuptools >=61 or better 62). I'd also try to use setuptool's mechanisms for reading the README, etc - the less custom code you have in setup.py, the better.

Of course, the simplest fix would be to remove the if statement.

PS: setuptools has deprecated running tests with setup.py, so you could at least drop your test command code.

@rogerbinns
Copy link
Author

I'll leave out the name == main bit and see what happens. Just found that CIBW_PROJECT_REQUIRES_PYTHON doc says this should work ..

@henryiii
Copy link
Contributor

Again, I'd highly recommend moving all the static config to setup.cfg (remove from setup call, add to setup.cfg). This makes parsing it much easier for tooling like cibuildwheel. But if you remove the if statement, it should work, and it's probably a cibuildwheel bug if it doesn't (parsing arbitrary AST is hard).

rogerbinns added a commit to rogerbinns/apsw that referenced this issue Sep 13, 2023
@rogerbinns rogerbinns changed the title setuptools/setup.py ignores python_requires setup.py setup() not detected in __name__ == '__main__' block Sep 13, 2023
@rogerbinns
Copy link
Author

Removing the if name == main did make cibuildwheel pick up the python_requires bit. It is going to cause me other problems because some of my other tooling uses functions from setup.py and is going to require some ugly monkey patching to prevent setup() from running. It would be really nice if cibuildwheel ast parsing allows the name == main if statement.

The test command code is not there for setuptools use, as in that is not the usual way tests are run. It is there as an arbitrary command to help with test configuration. For example a build option can say that a particular SQLite extension should be enabled, and when test runs from the same process it can verify that extension is actually present. When the test suite is run separately it auto-detects which extensions are present, but if there is a bug in setup.py where it is told to include an extension, and doesn't then that won't be caught.

Putting stuff in setup.cfg has problems like in #1487 and then means the information is now in two places. Additionally setup.cfg is also there for the end users and provides a place for them to specify additional compilation options such as defines and libraries. (As in a generic thing that has been part of distutils forever.)

I did try moving stuff to pyproject.toml [project] and it doesn't work because setuptools then starts complaining a lot, as I play a game of whack a mole moving the various keys and values.

The fundamental issue here is that cibuildwheel is not allowing for a name == main block containing the setup call. I'll rename the ticket. The suggested fixes are one of:

  • Close as wontfix
  • Close by documenting the name == main limitation in CIBW_PROJECT_REQUIRES_PYTHON section
  • Make name == main work

@henryiii
Copy link
Contributor

setup.cfg is for static project configuration, it's not something for users to write from scratch. See https://setuptools.pypa.io/en/latest/userguide/declarative_config.html, since 2016. Even before that, lots of tools supported adding configuration to setup.cfg. Even https://docs.python.org/3/distutils/configfile.html[^1] says installers can edit setup.cfg, not replace it with a new generated file that wipes out the current contents! Doing this will break many packages. The original design for arbitrary overrides was more in the "installers can override anything in setup.cfg using the command-line options to setup.py" part, IMO. There's also DIST_EXTRA_CONFIG which I think is closer to what you want for installer level configuration, but be warned cibuildwheel sets this to its own file when cross compiling on Windows to ARM.

No other package that I'm aware of uses setup.cfg like you are using it, while I can name over 52,000 packages that use it for static configuration.

Anyway, that's not really related to the issue here. IMO supporting __name__ == "__main__" is pretty reasonable, and I've attempted it in #1613.

@rogerbinns
Copy link
Author

What has happened with setup.cfg is that some folks have their own version with their own configuration for my extension such as SQLite settings. (My extension is 20 years old!) So when they build my extension they copy in their setup.cfg as they have been doing forever. If I move stuff from setup.py into setup.cfg then what they have done will break and cause them more work. That is okay, providing there are benefits, but I don't see any versus retaining it all in setup.py. Some of it has to be dynamic anyway (ie in code) because of figuring out compile time information.

I'm looking forward to name == main working. Thanks for doing that work.

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 a pull request may close this issue.

2 participants