-
Notifications
You must be signed in to change notification settings - Fork 369
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
[MRG/REVIEW] Support Pipfile / Pipfile.lock with pipenv #649
Conversation
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/an-unfolding-story-of-my-first-contribution-to-repo2docker/839/1 |
As setup.py can be explicitly installed or not from a Pipfile I reasoned that it should not be installed by default after pipenv has used relevant Pipfiles.
We want to ignore the setup.py file if it is around along with a Pipfile.
We want to use the more specific version, Pipfile.lock, if it is available. Are we ignoring the Pipfile if it is, as we should?
Apparently testing to import the package will work no matter what as we will find it locally so setup.py may not need to run. By adding a small dependency I could check if the dependency is installed and now the tests actually tests what they should.
Previously we did not check that requirements.txt was ignored properly, now we do.
Why install a big package when we can install a trivial one?
This is the actual meat while most previous commits were tests to verify that this worked as intented. The tests paid off quickly as this was quite hard to get working at all. - We can not use `pipenv shell` inside this script. - We must use `--system` and therefore `pipenv install` that supports it rather than `pipenv sync` that doesn. - We must generate a `Pipfile.lock` if there are none. - We must use `--ignore-pipfile` and `--deploy` on the `pipenv install` command in order to mimic how a `pipenv sync` would have worked.
Give priority to Pipfile.lock, then Pipfile, then runtime.txt.
I did a quick iteration on the documentation of Pipfiles - @betatim I think this PR may be close to complete now. Hmmm okay I realize there may be a changelog etc I should note things in. |
The '//' will render to '/' in the Dockerfile that allows the RUN command to continue on a new line. I initially added a '//' within a python string on a blank line in order to format things within the Python file to look potentially nicer, but this is of little importance and I have now removed it again.
Merging this now. 👏 for the stamina to see this through!! |
Wieee thanks for your excellent review @betatim ! ❤️ I merged it to mybinder.org deploy it and it works on mybinder.org now! |
Noice! Do you want to make a new repository on https://github.com/binder-examples/ that demos this? Then we can send a tweet out as well. |
@betatim yes good idea! Will do! |
yahoooo! |
[MRG/REVIEW] Support Pipfile / Pipfile.lock with pipenv
To help you review this PR
I've added a new buildpack, it will allow repo2docker to detect Pipfile / Pipfile.lock and use them to install Python packages with
pipenv
. Here is a repo that made me want this functionality.There are 48 files changed in this PR, but most of them are relating to tests of the functionality added. Other than these tests, we have changes to...
.gitignore - When running tests locally I ended up with some artifacts, this change helped me avoid these.
.travis.yml - I declared the need for another build job for our CI to run tests against this buildpack.
docs/source/config_files.rst - I documented the new build pack
repo2docker/app.py - I added an import statement, sorted the imports alphabetically, and added the buildpack in the ordered list that decides what buildpack has priority assuming it decides it can do something through the
detect
function a buildpack should have.repo2docker/buildpacks/init.py - Allow repo2docker.buildpacks module expose the new buildpack.
repo2docker/buildpacks/pipfile/init.py - The logic of the buildpack, this is the key part to review I think.
The rest of the files are for the tests within the
tests/pipfile
folder. I added quite a bit of tests I'd say!About
This PR aims to close #174. The goal is to make repo2docker consider
Pipfile
andPipfile.lock
dependency files usingpipenv
.Code strategy
The strategy adopted was to mimic sections for how a
requirements.txt
configuration file was treated but give these newPipfile.lock
orPipfile
files a higher precedence.Documented process
I wrote in this discourse topic as I was speaking to a rubber duck to help myself think clearly along the way.
What to review
The files
docs/source/config_files.rst
.repo2docker/buildpacks/python/__init__.py
. All three functions are modified a bit, namelydetect()
,python_version()
, andget_assemble_scripts()
./tests/venv/pipfile
..gitignore
file catching some scrap generated after running all thepytest
.Concerns
Not using PyEnv that is streamlined to work alongside Pipenv.
python_version()
that is overriding the CondaBuildPack's function and influences the installed Python version.python_full_version
specifications.Not entering
pipenv shell
, missing out on automatic loading of.env
files for example.Not managing to install dependencies in a
Pipfile
with Python 3.5 or lower.Opinionated
pipenv
will be used to do the installation as compared to using it to create arequirements.txt
file and then do the installation withpip
.environment.yml
,Pipfile.lock
,Pipfile
, andrequirements.txt
will be used to install packages.Pipfile.lock
,Pipfile
, andruntime.txt
will be used to decidepython_version()
.setup.py
is ignored if aPipfile(.lock)
exists, as they can declare such installations. This can for example be seen in this repo.Why didn't I use PyEnv?
pipenv install
would run that in turn triggers PyEnv to setup the right Python version, a Python version would have already been installed. So using PyEnv would have required deeper changes that I wasn't ready to tackle without better understanding about repo2docker.pip
or similar, but it required addingapt-get
packages etc. first which was a hassle.