search

MariaDB / mariadb_kernel

A MariaDB Jupyter kernel
BSD 3-Clause "New" or "Revised" License
30 stars 21 forks source link

Switch to setuptools + setuptools scm #1

Closed Shelnutt2 closed 4 years ago

Shelnutt2 commented 4 years ago

This PR switches the setup.py to use setuptools instead of distutils. Setuptools has several benefits including the ability to run python setup.py develop --user which is similar to pip install --editable . except its a little more robust by allowing egg linking instead of the easypath setup pip does.

Secondly I've included a switch to use setuptools-scm. This allows for automatic versioning based on the latest git tag. I've used this in many repositories and it often help to avoid the manual setting of versions. I've also included a .git_archival.txt and .gitattributes which allows setuptools-scm this repo to be used from github archive tarballs. This is often a setup used for conda packages if you are not building conda packages from pypi packages directly.

I've previously signed the MCA if that matters for this repository.

Shelnutt2 commented 4 years ago

@robertbindar when you install via pip, pip usually uses a temporary directory for the build. It is not done in place like when running setup.py. That is likely why in the repo folder you don't see the version file updated. Can you check the installed package? You should see the version set there.

robertbindar commented 4 years ago

Tried it and indeed _version.py is generated in the installed package. I guess until packaging is ready, we need to add one extra step in the README file so that people generate the _version.py file themselves, otherwise python would just get the modules from the local copy of the repository (where _version does not exist) and the kernel would crash. Does this sound ok to you?

Shelnutt2 commented 4 years ago

Tried it and indeed _version.py is generated in the installed package. I guess until packaging is ready, we need to add one extra step in the README file so that people generate the _version.py file themselves, otherwise python would just get the modules from the local copy of the repository (where _version does not exist) and the kernel would crash. Does this sound ok to you?

Do you mean if people are trying to run the python module from the repo directory? Since python prefers the local path over installed modules, a user needs to run python setup.py develop or python setup.py develop --user to build the modules inplace. Pip installation will not work if the intent is to run the from module directory. This is not just a problem with this PR but anything that needs to be "built" won't work with pip since its an out-of-tree build. When a user installs from pip they need to run python from a different directory than this repo.

I'm not apposed to changes, or adding the extra step to the readme. I just want to make sure we cover the intent use case so we make the user experience the clearest!

robertbindar commented 4 years ago

Tried it and indeed _version.py is generated in the installed package. I guess until packaging is ready, we need to add one extra step in the README file so that people generate the _version.py file themselves, otherwise python would just get the modules from the local copy of the repository (where _version does not exist) and the kernel would crash. Does this sound ok to you?

Do you mean if people are trying to run the python module from the repo directory? Since python prefers the local path over installed modules, a user needs to run python setup.py develop or python setup.py develop --user to build the modules inplace. Pip installation will not work if the intent is to run the from module directory. This is not just a problem with this PR but anything that needs to be "built" won't work with pip since its an out-of-tree build. When a user installs from pip they need to run python from a different directory than this repo.

Something like this, yes. So usually before this PR, the workflow to try the kernel was: clone the repo, run pip install ., install the kernelspec with python -m mariadb_kernel.install. With this PR, because python prefers the local path and because _version.py is not generated in the local path, python -m mariadb_kernel.install crashes when _version.py isn't found. But sure thing, python setup.py develop --user sounds like a good replacement instead of having 2 commands: python setup.py --version to generate _version.py and pip install .

Do you want to rebase the PR and I will update the README once we merge this? I can do the rebase no problem if you don't have the time.

I'm not apposed to changes, or adding the extra step to the readme. I just want to make sure we cover the intent use case so we make the user experience the clearest!

Shelnutt2 commented 4 years ago

@robertbindar Thank you for your feedback and discussion on the PR. I think using the python setup.py develop is a good solution here since it extents beyond just the version.py file but to any part of the build that is needed in the local directory for running. I've rebased it on the latest master, so it should be good now.

robertbindar commented 4 years ago

Thank you! It's been awesome working with you :sunglasses: