jupyter-remote-desktop-proxy icon indicating copy to clipboard operation
jupyter-remote-desktop-proxy copied to clipboard

Remove apt.txt, environment.yml, and refactor Dockerfile

Open consideRatio opened this issue 4 years ago • 8 comments

I'm trying to learn to use and contribute with maintenance of this project. Doing so, I'm looking at all files and trying to understand their purpose etc.

  • I noted that the apt.txt seemed outdated and no longer relevant as having a Dockerfile makes a binderhub ignore the apt.txt file, the same is true for the environment.yml file.
  • I noted that websockify was required but it wasn't listed in install_requires of setup.py, but installing it with pip didn't help - but instead one was required to install it with conda/mamba.
  • I hoped to put files that I believed were binderhub related into .binder/ but that wouldn't work, so I felt it was nice to embed the environment.yml file into the Dockerfile to minimize files in the root folder that increase the complexity.

consideRatio avatar Jul 25 '21 01:07 consideRatio

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

welcome[bot] avatar Jul 25 '21 01:07 welcome[bot]

Binder :point_left: Launch a binder notebook on this branch for commit 99543d98c5a5ec898dd10b733f1bd7986e8d2aa8

Erik note: this works!

github-actions[bot] avatar Jul 25 '21 01:07 github-actions[bot]

Binder :point_left: Launch a binder notebook on this branch for commit e5195e8068f5f67aed80cfba734eaa35e7cee6ce

github-actions[bot] avatar Jul 26 '21 23:07 github-actions[bot]

@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 dbus-x11 and xfce4-session it seems. I'm not sure about this, but to me, I ended up very confused no matter what.

consideRatio avatar Jul 26 '21 23:07 consideRatio

Binder :point_left: Launch a binder notebook on this branch for commit a7bb007c98657ace641fad54a8d28a69104e0eaa

github-actions[bot] avatar Jul 26 '21 23:07 github-actions[bot]

I added back environment.yml as dev-environment.yml to clarify it was not associated with the Dockerfile or the binderhub build, but as something for development. I also made it not explicitly install jupyter-server-proxy but letting it be installed via installing the local package instead.

Will the local package be installed in editable mode like this btw, as you probably want for local dev?

consideRatio avatar Jul 26 '21 23:07 consideRatio

I still think we should keep it as environment.yml since it's not just for dev, it's a bit in between. If you want we could put this PR on hold until I've dealt with https://github.com/jupyterhub/jupyter-remote-desktop-proxy/issues/12 ?

manics avatar Jul 27 '21 08:07 manics

If you want we could put this PR on hold until I've dealt with #12 ?

Sure!

consideRatio avatar Jul 27 '21 08:07 consideRatio

Binder :point_left: Launch a binder notebook on this branch for commit 9e1c0d8cfa30c55a0f81b143ed1b66c89f407d20

github-actions[bot] avatar Jan 14 '23 21:01 github-actions[bot]

@manics I just re-arrive to this repo and ended up wishing to update things that could come in conflict with this PR, can we go for a merge here without doing #12 first?

consideRatio avatar Jan 14 '23 21:01 consideRatio

Binder :point_left: Launch a binder notebook on this branch for commit a9fc17b96ffa5c513ada2ef5458350009dde3098

github-actions[bot] avatar Jan 14 '23 22:01 github-actions[bot]

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.

yuvipanda avatar Jan 14 '23 22:01 yuvipanda

I'm just going to merge this for now, as removing the apt.txt is definitely an improvement, and we could do further improvements after this. Hope that's ok, @manics

yuvipanda avatar Jan 14 '23 22:01 yuvipanda