Python bindings documentation issues

I went through the Python bindings docs on the website and catched two issues:

• The advocated variable -DPYTHON_INSTALL_LOCATION="user" is actually called DUNE_PYTHON_INSTALL_LOCATION
• The text advocates the use of a variable ADDITIONAL_PIP_PARAMS. However, to my knowledge, this variable does not exist. There exists a parameter of that name to dune_python_install_package, but there is no global switch. If such a switch is added, a consistent naming scheme would be DUNE_PYTHON_ADDITIONAL_PIP_PARAMS.
• The pip flag for upgrading is --upgrade not -upgrade

added doc label

changed the description

I have already started a cleanup in a MR: infrastructure/dune-website!363 (merged). Maybe you can add your suggestions there.

Some other parameters might be missing in the documentation, e.g. PYTHONPATH

Misread the issue with ADDITIONAL_PIP_PARAMS I need to check

As I wrote, this is a parameter to dune_python_install_package, but you document it as a global CMake option you would use as -DADDITIONAL_PIP_PARAMS=1. Only the function argument is considered in line 82.

I'll check why this seems to work. Without the --upgrade there was always an issue with calling make python_install after a change to the python scripts. Pip ignores a request for reinstall if the version number in setup.py doesn't change so calling make python_install would lead to no change without the --upgrade. Since I'm using editable I might not have noticed some change here - I know I used to test that reinstalling the python package with make worked.

I just ran a few checks: it seems that using the --upgrade is not needed anymore. I just checked and that is now always added to the pip command in dune-common/cmake/modules/DunePythonInstallPackage.cmake. So we can remove that parameter from the documentation.

One things I noticed while testing that is possibly due to the changes made for the Python packaging: without editable the running make install_python in dune-common fails first time because wheel is missing. I added it to the required packages then it works but the output is not great for users - don't know if that can be avoided:

Processing ./python
Collecting numpy
  Using cached numpy-1.19.5-cp38-cp38-manylinux2010_x86_64.whl (14.9 MB)
Collecting portalocker
  Using cached portalocker-2.1.0-py2.py3-none-any.whl (13 kB)
Collecting wheel
  Using cached wheel-0.36.2-py2.py3-none-any.whl (35 kB)
Building wheels for collected packages: dune-common
  Building wheel for dune-common (setup.py) ... error
  ERROR: Command errored out with exit status 1:
   command: /home/dedner/dunepy-test/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-req-build-nykhnvyr/setup.py'"'"'; __file__='"'"'/tmp/pip-req-build-nykhnvyr/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-jm4oj1i1
       cwd: /tmp/pip-req-build-nykhnvyr/
  Complete output (6 lines):
  usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: setup.py --help [cmd1 cmd2 ...]
     or: setup.py --help-commands
     or: setup.py cmd --help
  
  error: invalid command 'bdist_wheel'
  ----------------------------------------
  ERROR: Failed building wheel for dune-common
  Running setup.py clean for dune-common
Failed to build dune-common
Installing collected packages: numpy, portalocker, wheel, dune-common
    Running setup.py install for dune-common ... done
Successfully installed dune-common-2.8.0 numpy-1.19.5 portalocker-2.1.0 wheel-0.36.2
Built target install_python__home_dedner_DUNE_dune-common_python
Built target install_python

I think the issue you describe requires a pyproject.toml file: https://www.python.org/dev/peps/pep-0518/ These files describe requirements of the build process itself. I will try this later today.

I did think of that as well - we use that also for the Python packaging but didn't include it for this case. I'm just testing it so give it a moment.

This does solve it - in fact even having an empty pyroject.toml in common-build-dir/python solves the problem already. Don't know if that is a feature or ... I'll add this to the fix/setup-dunepy branch in dune-common

mentioned in merge request !918 (merged)

There is a flag ADDITIONAL_PIP_PARAMS in dune-common/cmake/modules/DunePythonInstallPackage.cmake which is passed on to the pip command in line 82. It needs to be set to --upgrade in our usage to allow for reinstalling changes to the python scripts without changing the version number each time.

I am repeating myself here, but the line you mention dereferences PYINST_ADDITIONAL_PIP_PARAMS not ADDITIONAL_PIP_PARAMS. PYINST_ADDITIONAL_PIP_PARAMS is the result of parsing the arguments to dune_python_install_package, which means that this option is only available for package developers, not to users as suggested in the documentation.

That was a 'reply by email' comment which I then pasted in to the gitlab and then removed again after I understood what you were talking about then Rene fixed the reply by email and so that it popped up again. I did understand what you were talking about....

Okay, cool.

mentioned in merge request !919 (merged)

I have implemented the behaviour you are interested in: !919 (merged) It actually is an interface that is worthwhile to consider e.g. to point pip to PyPI alternatives in restricted environments.

added python label

mentioned in issue #330

Seems resolved!

closed