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

Do not install required pip packages on behalf of users #935

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

ptheywood
Copy link
Member

@ptheywood ptheywood commented Oct 19, 2022

Emit a CMake Fatal Error during configuraition for missing required python packages, rather than installing them into the users environment.

Wheel, setuptools and optionall venv are requred.

Installing into --user automatically would fail in some cases, and installing not into user into a not-generated venv is bad form.

Closes #639

Also switches from the deprectaed setup.py install to use build --wheel.
Closes #947.

@ptheywood
Copy link
Member Author

Using a fresh venv on my ubuntu system, which doesn't contain wheel (but does contain setuptools and venv):

cmake .. -DBUILD_SWIG_PYTHON=ON

...

-- -----Configuring Project: pyflamegpu-----
-- pyflamegpu-2.0.0a3 (2.0.0a3+cuda118)
-- Found python module: setuptools (version "59.6.0")
CMake Error at swig/python/CMakeLists.txt:221 (message):
    Unable to find required python module "wheel".
    Please install this to your python environment, e.g.
      python3 -m pip install wheel
Call Stack (most recent call first):
  swig/python/CMakeLists.txt:231 (search_python_module)


-- Configuring incomplete, errors occurred!

After installing via python3 -m pip install wheel, it configures successfully.

Copy link
Member

@Robadob Robadob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible.

Would we benefit from a requirements.txt or similar?

@ptheywood
Copy link
Member Author

Would we benefit from a requirements.txt or similar?

We could, but things like venv are optional (and not always installable via pip). It would only contain wheel and setuptools, and I'd be reluctant to pin versions given the broad range of python version support (and subsequent dependabot spam).

Individual examples might then have additional dependencies, including pflamegpu but that is not installable via name via pip anyway, so would just cause use of requirements.txt to fail if included.

I'm inclined to leave it for the future (though it might not hurt to open an issue?)

Copy link
Member

@mondus mondus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a brief sentence in README.md about where cmake will search for python and that cmake can be run from inside a virtual environment with the dependencies installed. Seems like this PR would be a good place for this to go in as its adding python discrepancies.

Wheel, setuptools and optionall venv are requred.
Installing into --user automatically would fail in some cases, and installing not into user into a not-generated venv is bad form.

Closes #639
This is not an ideal location, but the best option without more signifcant change to the readme
@ptheywood ptheywood merged commit b82632d into master Oct 26, 2022
@ptheywood ptheywood deleted the no-pip-user-installs branch October 26, 2022 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setup.py install is deprecated Do not install pip packages into the current environment
3 participants