-
Notifications
You must be signed in to change notification settings - Fork 107
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
Remove apt.txt and refactor Dockerfile #13
Remove apt.txt and refactor Dockerfile #13
Conversation
This comment has been minimized.
This comment has been minimized.
👈 Launch a binder notebook on this branch for commit 99543d9 Erik note: this works! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to remove apt.txt.
Could we keep environment.yml
though? It's helpful for anyone who wants to test this outside Docker, and avoids mixing conda and pip packages. Maybe best to think of the Dockerfile as an example as well as an output artifact.
Unfortunately the version of websockify on PyPI doesn't include the compiled wrapper library which is used by this extension, so either you'd have to manually compile it after pip installing websockify, or use the conda package.
Co-authored-by: Simon Li <[email protected]>
👈 Launch a binder notebook on this branch for commit e5195e8 |
@manics hmmm okay we can add the environment.yml back, but without any instructions, I couldn't say it was part of what was required or not for the actual local dev etc or if it was something needed by the dockerfile only. In practice, the environment.yml only installs websockify besides the local python package, but besides having installed that, one need to install other things as well as i understand it, such as |
👈 Launch a binder notebook on this branch for commit a7bb007 |
I added back Will the local package be installed in |
I still think we should keep it as |
Sure! |
👈 Launch a binder notebook on this branch for commit 9e1c0d8 |
👈 Launch a binder notebook on this branch for commit a9fc17b |
I think the Dockerfile here should eventually lead towards something we can use to test changes to this repo. New users should probably steal from https://github.com/binder-templates/binder-desktop-app-template instead, although this repo itself needs more docs. |
I'm just going to merge this for now, as removing the |
The
apt.txt
file didn't serve a purpose with a Dockerfile next to it for binder, so I removed it to avoid causing confusion.The Dockerfile got itself an update, but needs even more updates still, for example so it can avoid introducing a lockscreen etc. I can iterate on that but I'd love to do that in a future PR instead of letting this linger.