search

lightning-power-users / node-launcher

Easiest Bitcoin Lightning desktop app, for Windows, macOS, and Linux
MIT License
357 stars 66 forks source link

Use pipfile.lock #91

Open PierreRochard opened 5 years ago

PierreRochard commented 5 years ago

https://twitter.com/jamesob/status/1077775521046511617?s=21

willcl-ark commented 5 years ago

Hi @PierreRochard I am happy to take this one up for you as a first PR to the project. The only potential issue that I see is that pipenv recommend only targeting one version of python:

Specify your target Python version in your Pipfile’s [requires] section. Ideally, you should only have one target Python version, as this is a deployment tool.

They double down quite hard on this in the project issues: e.g.: https://github.com/pypa/pipenv/issues/1050

I saw another issue on node-launcher which was trying to make the application python-agnostic (#87). I don't think the move to Pipfile.lock and making the application agnostic can work without extremely careful, and not worth it, consideration and maintenance. Which do you feel is more important here?

Interestingly, to me, in that discussion on the pipenv project issue which I linked above, they draw a hard distinction between an 'application' who will always be deployed in identical environments, which pipenv is suitable for, and 'libraries' (tools) who will be shared in many different environments, different python versions, for which they will not make pipenv suitable for...

PierreRochard commented 5 years ago

Thank you for doing the research @willcl-ark !

In that case I think we should always target the latest version of Python (3.7.2 right now).

I understand that some distros are behind on Python versions, but that's not really this project's problem.

willcl-ark commented 5 years ago

Hi @PierreRochard, sorry in advance for the long comment!

I have created a branch on my fork which removes the requirements.txt file and implements the more secure Pipfile system:

https://github.com/willcl-ark/node-launcher/tree/pipfile

I also updated the README.md to reflect the new installation instructions. As I mentioned above the project would now need a specific python version, python 3.7.2, which gives this system its deterministic properties.

Is it standard for me now to submit a pull request to your project for testing, or are you happy to clone and test directly form my branch?

One thing I would say on reflection on having done this is that it certainly seems like more effort than the requirements.txt approach at first (as well as being more pernickety about python versions etc.), but I can also appreciate the extra security you do get from it once it's up and running. At the end its quite easy, it simply replaces pip in your project completely (after you use pip once to install it).

Now that I fully have my head around it I think I see why it is superior to pip, especially if you are using pyenv to manage and automatically install python versions on osx (install from homebrew!). If you are using pyenv, then when cloning a project with a Pipfile requiring a certain python version, pipenv will call pyenv and automatically install the required python version in a new venv for you -- pretty cool!

Any questions let me know, I spent about 7 hours today troubleshooting various parts of it so feel like I could explain any questions you might have pretty well!

willcl-ark commented 5 years ago

Also of note, you can split packages into 'normal' and ones only required for 'dev' work. I moved the two pytest ones into 'dev' (which is why you install with pipenv install --dev), so either it could continue with a split (perhaps other packs could move into 'dev' section too?) or we could just keep them all in 'normal' which means we wouldn't have to use the dev flag when setting up dev environment (might be cleaner)

PierreRochard commented 5 years ago

Fantastic work @willcl-ark, thank you! Please submit a pull request for testing and merging

Splitting is great, thank you!