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

get_requires_for_build_wheel confuses tox4 by not requesting cmake if it is in the build venv #458

Closed
bennyrowland opened this issue Aug 11, 2023 · 1 comment · Fixed by #462

Comments

@bennyrowland
Copy link
Collaborator

I just moved a project from old scikit-build and tox3 to scikit-build-core and tox4 - the improved caching on both sides has taken my test time from several minutes to several seconds, so very happy about that. Unfortunately, scikit-build-core's clever use of get_requires_for_build_wheel to add cmake etc. only when they are not already available is backfiring somewhat.

The first run works perfectly, with tox installing cmake in the build environment. For the second run scikit-build-core finds that cmake is available in its path and so does not request it in get_requires_for_build_wheel(). tox sees that the requirements have changed, and scraps the build env (and promptly crashes, but that is a secondary issue, I think). So I think that a better solution would be for scikit-build-core to check if cmake is available through the env (rather than anywhere else) and if so then it should continue to say that it requires the package. Since the package is already installed, it won't make a material difference to the run time of anything for pip to decide it doesn't need to do anything, but it will stop tox from getting confused about the changing requirements. Note that if I add cmake to the list of build requirements in pyproject.toml (so scikit-build-core never asks for it and never changes its mind) then the problem goes away.

I guess that there are some technical questions about deciding whether cmake is coming from the environment, but the simplest way is probably just to import cmake and check for an error. This could be a short-circuit at the top of GetRequires.cmake(). If cmake can be imported then yield f"cmake>={cmake_min}". @henryiii I am happy to submit a PR if this change meets with your approval.

@henryiii
Copy link
Collaborator

henryiii commented Aug 11, 2023

I'd say adding cmake and/or ninja if the package is installed is perfectly fine. I'd only point out that the check should be via importlib.util.find_spec("cmake") / importlib.util.find_spec("ninja") is not None - it's cheaper and doesn't require squelching linter warnings. I don't think it's even necessary to test the version; if the cmake package is present, the requirement is always fine to add.

henryiii added a commit that referenced this issue Aug 14, 2023
Fix #458.

@bennyrowland, I think this either fixes this or is a good start.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
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