Patching setup.py for installation without pip for Spack

[ajaust]

For the installation of the bindings with Spack I have patched setup.py. The Spack package (spack/spack#19558) uses the setup.py directly instead of using pip.

However, for checking the version I use a crude fix by importing from setuptools._vendor.packaging. I had problems when using the packaging package from Spack.

[ajaust]

When I add py-packaging explicitly to the Spack-recipe, I end up with the following error

==> py-pyprecice: Executing phase: 'build_ext'
==> [2020-10-28-11:22:35.261548] '/home/jaustar/software/spack/spack-fork/opt/spack/linux-ubuntu18.04-skylake/gcc-7.5.0/python-3.8.6-c5r3eojydom26iptzff22kt3hva5gep4/bin/python3.8' '-s' 'setup.py' '--no-user-cfg' 'build_ext' '--include-dirs=/home/jaustar/software/spack/spack-fork/opt/spack/linux-ubuntu18.04-skylake/gcc-7.5.0/precice-2.0.2-zxbytdw2k5ofq6ju5k434iv4zidfj4vd/include' '--library-dirs=/home/jaustar/software/spack/spack-fork/opt/spack/linux-ubuntu18.04-skylake/gcc-7.5.0/precice-2.0.2-zxbytdw2k5ofq6ju5k434iv4zidfj4vd/lib'
Traceback (most recent call last):
  File "setup.py", line 4, in <module>
    from packaging import version
ModuleNotFoundError: No module named 'packaging'

Branch with that approack (using packaging) can be found here: https://github.com/ajaust/spack/tree/version-from-packaging

[BenjaminRodenberg]

To summarize: With the suggested changes I think we are close to merging this PR. If necessary, we can open another one, if the packaging issue cannot be resolved on the spack side.

[ajaust]

I started to wonder whether the pip version should be checked in the setup.py at all. The minimum required version of pip should probably stated clearly in the installation instructions and that's it. There does not seem to exist any standard way to check whether the installation is triggered by pip or any other tool as far as I know. This leads me to the conclusion that there should not be any dependency on a tool like pip in the setup.py.

[BenjaminRodenberg]

What was the current status of this PR? As far as I remember it's ready to merge. @ajaust Or are there any open points? If not, I will approve and merge.

[ajaust]

I am not sure. How small should the fixes be?

The PR relaxes the dependency of pip. This might be done in a non-standard/unstable manner via if "pip" in __file__: so I am not sure if I am a fan of that. This might give us trouble some later time. However, I have no idea how Python packages solve this problem normally. At the same time there is work going on in #64 so I lost a bit track what part of setup.py should be fixed where and when.

Regarding the Spack installation this is only a small step for full Spack support. At the moment we still need the patch and a adapted installation procedure for the Spack package since:

• from packaging import version does not work and needs to be removed when using Spack (thus a patch is needed anyway).
• The current setup.py is lacking some options Spack expects such that we need to overwrite the Spack installation routine of the py-pyprecice package (setup.py does not provide option --single-version-externally-managed #64).

As the Spack package needs a patch anyway, we could also change the patch in the Spack repository to remove the whole pip related procedure. I am unsure what would be the best approach

1. Fixing this small part here and adapting the patch in the Spack repository accordingly.
2. Leaving the setup.py as it is at the moment and fix it properly such that we do not need the patch in the Spack repository anymore and such that we can use the standard installation procedure of Spack.

[BenjaminRodenberg]

Thanks for the summary!

The PR relaxes the dependency of pip. This might be done in a non-standard/unstable manner via if "pip" in __file__: so I am not sure if I am a fan of that. This might give us trouble some later time. However, I have no idea how Python packages solve this problem normally.

Relaxing the dependency on pip is a good thing, since this will allow us reduce the code that needs a patch. The non-standard if "pip" in __file__: is a bit a downside, but I would suggest to accept it, since we currently do not see a better solution.

Comparing the two options that we currently have (dependency on pip vs. non-standard check for pip), I think the latter is preferable and therefore would like to merge this PR. Ok?

At the same time there is work going on in #64 so I lost a bit track what part of setup.py should be fixed where and when.

Regarding the Spack installation this is only a small step for full Spack support. At the moment we still need the patch and a adapted installation procedure for the Spack package since:

• from packaging import version does not work and needs to be removed when using Spack (thus a patch is needed anyway).

I think that here the patch that we provide is a good solution: Using packaging is the best solution that unfortunately does not work on spack. So patching to use from setuptools._vendor.packaging import version it is a good workaround and the only option we have.

At the current state we do not have to do anything / cannot do anything here.

• The current setup.py is lacking some options Spack expects such that we need to overwrite the Spack installation routine of the py-pyprecice package (setup.py does not provide option --single-version-externally-managed #64).

Here, #65 will hopefully converge at some point. Right now, we are a bit stuck in this PR, but might occasionally find a solution.

As the Spack package needs a patch anyway, we could also change the patch in the Spack repository to remove the whole pip related procedure. I am unsure what would be the best approach

1. Fixing this small part here and adapting the patch in the Spack repository accordingly.

2. Leaving the setup.py as it is at the moment and fix it properly such that we do not need the patch in the Spack repository anymore and such that we can use the standard installation procedure of Spack.

I assume that we will still need a patch (at least due to packaging). #65 is also still open. Let's hope that we will be able to solve this, but right now I cannot really estimate this.

My suggestion: Let's merge this PR and create a draft PR to update the patch accordingly.

[BenjaminRodenberg]

I did a few checks with older pip version (pip 9.0..2) and using python3 setup.py install. All warnings are issued correctly. However, there is a downside: pip seems to only show warnings, if the -v option is provided.

Maybe we are really following a wrong approach here? If the user does not see the warning in the default case, it is not worth a lot to provide a warning.

[BenjaminRodenberg]

The latest commit introduces a fundamental change: pip 19.0.0 is now the minimal requirement. This allows us to raise an exception, if the version is too low and this is also something the user will see.

[BenjaminRodenberg]

From my point of view the new, stricter requirements on the pip version are reasonable and guarantee safe usage.

[BenjaminRodenberg]

@fsimonis can you take a look at this PR? Especially w.r.t code sanity and documentation of the installation process, requirements and dependencies?

[BenjaminRodenberg]

I merged the current state of develop into this fork. From my perspective everything now looks clean. I would like to merge this PR soon and then continue with #70.

[BenjaminRodenberg]

Already merged #70, since this will allow us to relax on the packaging dependency here. I will merge develop into this PR now and then merge #63.